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

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@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
 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)
+		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;
+
+			nexthdr = hp->nexthdr;
+			off += sizeof(struct frag_hdr);
+			break;
+		}
 		default:
 			done = true;
 			break;
-- 
1.7.10.4

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@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> ---
>  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

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 28 November 2013 13:13
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; Wei Liu; Ian Campbell; David Vrabel
> Subject: Re: [PATCH net] xen-netback: fix fragment detection in checksum
> setup
> 
> You seem to have forgotten to Cc netdev@.
> 
No, that was deliberate: I thought first review was supposed to be limited to
xen-devel. Is that not the case any more?

  Paul
> 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@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: David Vrabel <david.vrabel@citrix.com>
> > ---
> >  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

On Thu, Nov 28, 2013 at 01:13:40PM +0000, Paul Durrant
wrote:
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > Sent: 28 November 2013 13:13
> > To: Paul Durrant
> > Cc: xen-devel@lists.xen.org; Wei Liu; Ian Campbell; David Vrabel
> > Subject: Re: [PATCH net] xen-netback: fix fragment detection in
checksum
> > setup
> > 
> > You seem to have forgotten to Cc netdev@.
> > 
> 
> No, that was deliberate: I thought first review was supposed to be limited
to xen-devel. Is that not the case any more?
> 
I think this works too. There''s no guideline for this. The downside is
that if you post something like ''V2'' to netdev@ people there
might
wonder where V1 has gone. ;-)

Wei.

On Thu, 2013-11-28 at 13:13 +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > Sent: 28 November 2013 13:13
> > To: Paul Durrant
> > Cc: xen-devel@lists.xen.org; Wei Liu; Ian Campbell; David Vrabel
> > Subject: Re: [PATCH net] xen-netback: fix fragment detection in
checksum
> > setup
> > 
> > You seem to have forgotten to Cc netdev@.
> > 
> 
> No, that was deliberate: I thought first review was supposed to be
> limited to xen-devel. Is that not the case any more?
For stuff which is big an complex or adds new protocol features I think
discussing it on xen-devel and agreeing on the interface etc before
taking it to netdev makes sense, simply because 99% of the folks there
don''t care one jot and don''t want to read some enormous thread
where we
nitpick the name of the xenbus feature node ;-).

Obviously trivial stuff can go straight to netdev.

This one is somewhere in the middle and is a bit of a judgement call,
but I think erring on the side of xen-devel only first is fine.

Ian.