Re: [Xen-devel] [PATCH net] xen-netback: fix fragment detection in checksum setup

[Wei Liu <wei.liu2@xxxxxxxxxx>]

You seem to have forgotten to Cc netdev@.

On Thu, Nov 28, 2013 at 12:40:20PM +0000, Paul Durrant wrote:
> The code to detect fragments in checksum_setup() was missing for IPv4 and
> too eager for IPv6. (It transpires that Windows seems to send IPv6 packets
> with a fragment header even if they are not a fragment - i.e. offset is zero,
> and M bit is not set).
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
>  drivers/net/xen-netback/netback.c |   31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index 919b650..eea7ff2 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1165,15 +1165,27 @@ static int checksum_setup_ip(struct xenvif *vif, 
> struct sk_buff *skb,
>       struct iphdr *iph = (void *)skb->data;
>       unsigned int header_size;
>       unsigned int off;
> +     bool fragment;
>       int err = -EPROTO;
>  
> +     fragment = false;
> +
>       off = sizeof(struct iphdr);
>  
>       header_size = skb->network_header + off + MAX_IPOPTLEN;
>       maybe_pull_tail(skb, header_size);
>  
> +     if (ntohs(iph->frag_off) & 0x3fff)

I'm actually quite suprised that these magic numbers don't have a #define
in network core code.

0x3fff, if I'm not mistaken, is to check if a) M bit is set b) frag
offset is not zero. I think this warrants a line of comment.

> +             fragment = true;
> +
>       off = iph->ihl * 4;
>  
> +     if (fragment) {
> +             if (net_ratelimit())
> +                     netdev_err(vif->dev, "Packet is a fragment!\n");
> +             goto out;
> +     }
> +
>       switch (iph->protocol) {
>       case IPPROTO_TCP:
>               if (!skb_partial_csum_set(skb, off,
> @@ -1237,6 +1249,7 @@ static int checksum_setup_ipv6(struct xenvif *vif, 
> struct sk_buff *skb,
>       bool fragment;
>       bool done;
>  
> +     fragment = false;
>       done = false;
>  
>       off = sizeof(struct ipv6hdr);
> @@ -1275,9 +1288,21 @@ static int checksum_setup_ipv6(struct xenvif *vif, 
> struct sk_buff *skb,
>                       off += (hp->hdrlen+2)<<2;
>                       break;
>               }
> -             case IPPROTO_FRAGMENT:
> -                     fragment = true;
> -                     /* fall through */
> +             case IPPROTO_FRAGMENT: {
> +                     struct frag_hdr *hp = (void *)(skb->data + off);
> +
> +                     header_size = skb->network_header +
> +                             off +
> +                             sizeof(struct frag_hdr);
> +                     maybe_pull_tail(skb, header_size);
> +
> +                     if (ntohs(hp->frag_off) & 0xFFF9)
> +                             fragment = true;
> +

Same here, would be helpful if you add comment to clarify what 0xFFF9
is.

Wei.

> +                     nexthdr = hp->nexthdr;
> +                     off += sizeof(struct frag_hdr);
> +                     break;
> +             }
>               default:
>                       done = true;
>                       break;
> -- 
> 1.7.10.4

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel