* [PATCH net v2] xen-netback: fix fragment detection in checksum setup
@ 2013-11-28 13:23 Paul Durrant
2013-11-28 17:29 ` Eric Dumazet
2013-11-29 5:35 ` annie li
0 siblings, 2 replies; 6+ messages in thread
From: Paul Durrant @ 2013-11-28 13:23 UTC (permalink / raw)
To: xen-devel, netdev; +Cc: Paul Durrant, Wei Liu, Ian Campbell, David Vrabel
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>
---
v2
- Added comments noting what fragment/offset masks mean
drivers/net/xen-netback/netback.c | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 919b650..c7464d8 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1165,15 +1165,28 @@ 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);
+ /* 3fff -> fragment offset != 0 OR more fragments */
+ 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 +1250,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 +1289,22 @@ 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);
+
+ /* fff9 -> fragment offset != 0 OR more fragments */
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net v2] xen-netback: fix fragment detection in checksum setup
2013-11-28 13:23 [PATCH net v2] xen-netback: fix fragment detection in checksum setup Paul Durrant
@ 2013-11-28 17:29 ` Eric Dumazet
2013-11-29 10:32 ` Paul Durrant
2013-11-29 5:35 ` annie li
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-11-28 17:29 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, Wei Liu, Ian Campbell, David Vrabel
On Thu, 2013-11-28 at 13:23 +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).
> + /* 3fff -> fragment offset != 0 OR more fragments */
> + if (ntohs(iph->frag_off) & 0x3fff)
> + fragment = true;
> +
What about the more self documented and faster :
if (iph->frag_off & htons(IP_OFFSET | IP_MF))
fragment = true;
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH net v2] xen-netback: fix fragment detection in checksum setup
2013-11-28 17:29 ` Eric Dumazet
@ 2013-11-29 10:32 ` Paul Durrant
0 siblings, 0 replies; 6+ messages in thread
From: Paul Durrant @ 2013-11-29 10:32 UTC (permalink / raw)
To: Eric Dumazet
Cc: xen-devel@lists.xen.org, netdev@vger.kernel.org, Wei Liu,
Ian Campbell, David Vrabel
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: 28 November 2013 17:29
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Ian Campbell;
> David Vrabel
> Subject: Re: [PATCH net v2] xen-netback: fix fragment detection in checksum
> setup
>
> On Thu, 2013-11-28 at 13:23 +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).
>
> > + /* 3fff -> fragment offset != 0 OR more fragments */
> > + if (ntohs(iph->frag_off) & 0x3fff)
> > + fragment = true;
> > +
>
> What about the more self documented and faster :
>
> if (iph->frag_off & htons(IP_OFFSET | IP_MF))
> fragment = true;
>
Nicer definitely. I'll need to add the equivalent def for IP6_OFFSET to use the same style of test there.
Thanks,
Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] xen-netback: fix fragment detection in checksum setup
2013-11-28 13:23 [PATCH net v2] xen-netback: fix fragment detection in checksum setup Paul Durrant
2013-11-28 17:29 ` Eric Dumazet
@ 2013-11-29 5:35 ` annie li
2013-11-29 9:10 ` Paul Durrant
1 sibling, 1 reply; 6+ messages in thread
From: annie li @ 2013-11-29 5:35 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, Wei Liu, Ian Campbell, David Vrabel
On 2013/11/28 21:23, 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>
> ---
> v2
>
> - Added comments noting what fragment/offset masks mean
>
> drivers/net/xen-netback/netback.c | 33 ++++++++++++++++++++++++++++++---
> 1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 919b650..c7464d8 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1165,15 +1165,28 @@ 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;
Is it better to initialize fragment directly as following?
bool fragment = false;
> +
> off = sizeof(struct iphdr);
>
> header_size = skb->network_header + off + MAX_IPOPTLEN;
> maybe_pull_tail(skb, header_size);
>
> + /* 3fff -> fragment offset != 0 OR more fragments */
> + 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 +1250,7 @@ static int checksum_setup_ipv6(struct xenvif *vif, struct sk_buff *skb,
> bool fragment;
> bool done;
>
> + fragment = false;
> done = false;
Same as above for "done" and "fragment"...
Thanks
Annie
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH net v2] xen-netback: fix fragment detection in checksum setup
2013-11-29 5:35 ` annie li
@ 2013-11-29 9:10 ` Paul Durrant
2013-11-29 9:57 ` [Xen-devel] " annie li
0 siblings, 1 reply; 6+ messages in thread
From: Paul Durrant @ 2013-11-29 9:10 UTC (permalink / raw)
To: annie li
Cc: xen-devel@lists.xen.org, netdev@vger.kernel.org, Wei Liu,
Ian Campbell, David Vrabel
> -----Original Message-----
> From: annie li [mailto:annie.li@oracle.com]
> Sent: 29 November 2013 05:36
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Ian Campbell;
> David Vrabel
> Subject: Re: [PATCH net v2] xen-netback: fix fragment detection in checksum
> setup
>
>
> On 2013/11/28 21:23, 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>
> > ---
> > v2
> >
> > - Added comments noting what fragment/offset masks mean
> >
> > drivers/net/xen-netback/netback.c | 33
> ++++++++++++++++++++++++++++++---
> > 1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> netback/netback.c
> > index 919b650..c7464d8 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -1165,15 +1165,28 @@ 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;
>
> Is it better to initialize fragment directly as following?
> bool fragment = false;
>
I think that's a matter of personal taste. I tend to favour this style.
Paul
> > +
> > off = sizeof(struct iphdr);
> >
> > header_size = skb->network_header + off + MAX_IPOPTLEN;
> > maybe_pull_tail(skb, header_size);
> >
> > + /* 3fff -> fragment offset != 0 OR more fragments */
> > + 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 +1250,7 @@ static int checksum_setup_ipv6(struct xenvif *vif,
> struct sk_buff *skb,
> > bool fragment;
> > bool done;
> >
> > + fragment = false;
> > done = false;
>
> Same as above for "done" and "fragment"...
>
> Thanks
> Annie
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Xen-devel] [PATCH net v2] xen-netback: fix fragment detection in checksum setup
2013-11-29 9:10 ` Paul Durrant
@ 2013-11-29 9:57 ` annie li
0 siblings, 0 replies; 6+ messages in thread
From: annie li @ 2013-11-29 9:57 UTC (permalink / raw)
To: Paul Durrant
Cc: netdev@vger.kernel.org, David Vrabel, Wei Liu, Ian Campbell,
xen-devel@lists.xen.org
On 2013/11/29 17:10, Paul Durrant wrote:
>> -----Original Message-----
>> From: annie li [mailto:annie.li@oracle.com]
>> Sent: 29 November 2013 05:36
>> To: Paul Durrant
>> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Ian Campbell;
>> David Vrabel
>> Subject: Re: [PATCH net v2] xen-netback: fix fragment detection in checksum
>> setup
>>
>>
>> On 2013/11/28 21:23, 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>
>>> ---
>>> v2
>>>
>>> - Added comments noting what fragment/offset masks mean
>>>
>>> drivers/net/xen-netback/netback.c | 33
>> ++++++++++++++++++++++++++++++---
>>> 1 file changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
>> netback/netback.c
>>> index 919b650..c7464d8 100644
>>> --- a/drivers/net/xen-netback/netback.c
>>> +++ b/drivers/net/xen-netback/netback.c
>>> @@ -1165,15 +1165,28 @@ 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;
>> Is it better to initialize fragment directly as following?
>> bool fragment = false;
>>
> I think that's a matter of personal taste. I tend to favour this style.
>
That is OK, I point this out because it is inconsistent with other
variable initialization in netback.c.
Thanks
Annie
>
>>> +
>>> off = sizeof(struct iphdr);
>>>
>>> header_size = skb->network_header + off + MAX_IPOPTLEN;
>>> maybe_pull_tail(skb, header_size);
>>>
>>> + /* 3fff -> fragment offset != 0 OR more fragments */
>>> + 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 +1250,7 @@ static int checksum_setup_ipv6(struct xenvif *vif,
>> struct sk_buff *skb,
>>> bool fragment;
>>> bool done;
>>>
>>> + fragment = false;
>>> done = false;
>> Same as above for "done" and "fragment"...
>>
>> Thanks
>> Annie
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-29 10:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-28 13:23 [PATCH net v2] xen-netback: fix fragment detection in checksum setup Paul Durrant
2013-11-28 17:29 ` Eric Dumazet
2013-11-29 10:32 ` Paul Durrant
2013-11-29 5:35 ` annie li
2013-11-29 9:10 ` Paul Durrant
2013-11-29 9:57 ` [Xen-devel] " annie li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).