* [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank. @ 2011-01-05 13:23 Ian Campbell 2011-01-11 11:46 ` Ian Campbell 2011-01-22 0:58 ` Jeremy Fitzhardinge 0 siblings, 2 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-05 13:23 UTC (permalink / raw) To: netdev; +Cc: Ian Campbell, Jeremy Fitzhardinge, xen-devel The Linux network stack expects all GSO SKBs to have ip_summed == CHECKSUM_PARTIAL (which implies that the frame contains a partial checksum) and the Xen network ring protocol similarly expects an SKB which has GSO set to also have NETRX_csum_blank (which also implies a partial checksum). Therefore drop such frames on receive otherwise they will trigger the warning in skb_gso_segment. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: xen-devel@lists.xensource.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index cdbeec9..8b8c480 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -836,6 +836,11 @@ static int handle_incoming_queue(struct net_device *dev, dev->stats.rx_errors++; continue; } + } else if (skb_is_gso(skb)) { + kfree_skb(skb); + packets_dropped++; + dev->stats.rx_errors++; + continue; } dev->stats.rx_packets++; -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-05 13:23 [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank Ian Campbell @ 2011-01-11 11:46 ` Ian Campbell 2011-01-22 0:58 ` Jeremy Fitzhardinge 1 sibling, 0 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-11 11:46 UTC (permalink / raw) To: netdev, David Miller; +Cc: Jeremy Fitzhardinge, xen-devel Hi David, http://patchwork.ozlabs.org/patch/77593/ tells me this patch is "Not Applicable". Is this scenario not worth worrying about for some reason? The error would be due to a buggy peer (i.e. netback) so I guess this frontend fix is really just a belt-and-braces thing. However The equivalent netback patch (which is not upstream yet but I'm working on cleaning it up for a first post soon) is more critical since it could allow a malicious guest to spam the domain 0 syslog (via the WARN_ON in skb_gso_segment) so I just wanted to check if I was also missing some reason why the netback patch would be non-applicable too. Thanks, Ian. On Wed, 2011-01-05 at 13:23 +0000, Ian Campbell wrote: > The Linux network stack expects all GSO SKBs to have ip_summed == > CHECKSUM_PARTIAL (which implies that the frame contains a partial > checksum) and the Xen network ring protocol similarly expects an SKB > which has GSO set to also have NETRX_csum_blank (which also implies a > partial checksum). Therefore drop such frames on receive otherwise > they will trigger the warning in skb_gso_segment. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > Cc: xen-devel@lists.xensource.com > Cc: netdev@vger.kernel.org > --- > drivers/net/xen-netfront.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index cdbeec9..8b8c480 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -836,6 +836,11 @@ static int handle_incoming_queue(struct net_device *dev, > dev->stats.rx_errors++; > continue; > } > + } else if (skb_is_gso(skb)) { > + kfree_skb(skb); > + packets_dropped++; > + dev->stats.rx_errors++; > + continue; > } > > dev->stats.rx_packets++; ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-05 13:23 [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank Ian Campbell 2011-01-11 11:46 ` Ian Campbell @ 2011-01-22 0:58 ` Jeremy Fitzhardinge 2011-01-22 9:43 ` Ian Campbell 1 sibling, 1 reply; 15+ messages in thread From: Jeremy Fitzhardinge @ 2011-01-22 0:58 UTC (permalink / raw) To: Ian Campbell; +Cc: netdev, xen-devel On 01/05/2011 05:23 AM, Ian Campbell wrote: > The Linux network stack expects all GSO SKBs to have ip_summed == > CHECKSUM_PARTIAL (which implies that the frame contains a partial > checksum) and the Xen network ring protocol similarly expects an SKB > which has GSO set to also have NETRX_csum_blank (which also implies a > partial checksum). Therefore drop such frames on receive otherwise > they will trigger the warning in skb_gso_segment. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > Cc: xen-devel@lists.xensource.com > Cc: netdev@vger.kernel.org > --- > drivers/net/xen-netfront.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index cdbeec9..8b8c480 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -836,6 +836,11 @@ static int handle_incoming_queue(struct net_device *dev, > dev->stats.rx_errors++; > continue; > } > + } else if (skb_is_gso(skb)) { > + kfree_skb(skb); > + packets_dropped++; > + dev->stats.rx_errors++; > + continue; This looks redundant; why not something like: diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 47e6a71..c1b8f64 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -852,13 +852,12 @@ static int handle_incoming_queue(struct net_device *dev, /* Ethernet work: Delayed to here as it peeks the header. */ skb->protocol = eth_type_trans(skb, dev); - if (skb->ip_summed == CHECKSUM_PARTIAL) { - if (skb_checksum_setup(skb)) { - kfree_skb(skb); - packets_dropped++; - dev->stats.rx_errors++; - continue; - } + if (skb->ip_summed != CHECKSUM_PARTIAL || + skb_checksum_setup(skb)) { + kfree_skb(skb); + packets_dropped++; + dev->stats.rx_errors++; + continue; } dev->stats.rx_packets++; Thanks, J ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-22 0:58 ` Jeremy Fitzhardinge @ 2011-01-22 9:43 ` Ian Campbell 2011-01-24 17:55 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 15+ messages in thread From: Ian Campbell @ 2011-01-22 9:43 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: netdev, xen-devel On Sat, 2011-01-22 at 00:58 +0000, Jeremy Fitzhardinge wrote: > On 01/05/2011 05:23 AM, Ian Campbell wrote: > > The Linux network stack expects all GSO SKBs to have ip_summed == > > CHECKSUM_PARTIAL (which implies that the frame contains a partial > > checksum) and the Xen network ring protocol similarly expects an SKB > > which has GSO set to also have NETRX_csum_blank (which also implies a > > partial checksum). Therefore drop such frames on receive otherwise > > they will trigger the warning in skb_gso_segment. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > > Cc: xen-devel@lists.xensource.com > > Cc: netdev@vger.kernel.org > > --- > > drivers/net/xen-netfront.c | 5 +++++ > > 1 files changed, 5 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > index cdbeec9..8b8c480 100644 > > --- a/drivers/net/xen-netfront.c > > +++ b/drivers/net/xen-netfront.c > > @@ -836,6 +836,11 @@ static int handle_incoming_queue(struct net_device *dev, > > dev->stats.rx_errors++; > > continue; > > } > > + } else if (skb_is_gso(skb)) { > > + kfree_skb(skb); > > + packets_dropped++; > > + dev->stats.rx_errors++; > > + continue; > > This looks redundant; why not something like: > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 47e6a71..c1b8f64 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -852,13 +852,12 @@ static int handle_incoming_queue(struct net_device *dev, > /* Ethernet work: Delayed to here as it peeks the header. */ > skb->protocol = eth_type_trans(skb, dev); > > - if (skb->ip_summed == CHECKSUM_PARTIAL) { > - if (skb_checksum_setup(skb)) { > - kfree_skb(skb); > - packets_dropped++; > - dev->stats.rx_errors++; > - continue; > - } > + if (skb->ip_summed != CHECKSUM_PARTIAL || > + skb_checksum_setup(skb)) { That drops non-partial skbs. However they are fine unless they also claim to be gso. Perhaps you meant "skb->ip_summed == CHECKSUM_PARTIAL && ! skb_checksum_setup(skb)" which I think works but doesn't allow us to correctly chain the gso check onto the else. Ian. > + kfree_skb(skb); > + packets_dropped++; > + dev->stats.rx_errors++; > + continue; > } > > dev->stats.rx_packets++; > > Thanks, > J > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-22 9:43 ` Ian Campbell @ 2011-01-24 17:55 ` Jeremy Fitzhardinge 2011-01-25 17:09 ` Ian Campbell 0 siblings, 1 reply; 15+ messages in thread From: Jeremy Fitzhardinge @ 2011-01-24 17:55 UTC (permalink / raw) To: Ian Campbell; +Cc: netdev, xen-devel On 01/22/2011 01:43 AM, Ian Campbell wrote: > On Sat, 2011-01-22 at 00:58 +0000, Jeremy Fitzhardinge wrote: >> On 01/05/2011 05:23 AM, Ian Campbell wrote: >>> The Linux network stack expects all GSO SKBs to have ip_summed == >>> CHECKSUM_PARTIAL (which implies that the frame contains a partial >>> checksum) and the Xen network ring protocol similarly expects an SKB >>> which has GSO set to also have NETRX_csum_blank (which also implies a >>> partial checksum). Therefore drop such frames on receive otherwise >>> they will trigger the warning in skb_gso_segment. >>> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>> Cc: Jeremy Fitzhardinge <jeremy@goop.org> >>> Cc: xen-devel@lists.xensource.com >>> Cc: netdev@vger.kernel.org >>> --- >>> drivers/net/xen-netfront.c | 5 +++++ >>> 1 files changed, 5 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c >>> index cdbeec9..8b8c480 100644 >>> --- a/drivers/net/xen-netfront.c >>> +++ b/drivers/net/xen-netfront.c >>> @@ -836,6 +836,11 @@ static int handle_incoming_queue(struct net_device *dev, >>> dev->stats.rx_errors++; >>> continue; >>> } >>> + } else if (skb_is_gso(skb)) { >>> + kfree_skb(skb); >>> + packets_dropped++; >>> + dev->stats.rx_errors++; >>> + continue; >> This looks redundant; why not something like: >> >> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c >> index 47e6a71..c1b8f64 100644 >> --- a/drivers/net/xen-netfront.c >> +++ b/drivers/net/xen-netfront.c >> @@ -852,13 +852,12 @@ static int handle_incoming_queue(struct net_device *dev, >> /* Ethernet work: Delayed to here as it peeks the header. */ >> skb->protocol = eth_type_trans(skb, dev); >> >> - if (skb->ip_summed == CHECKSUM_PARTIAL) { >> - if (skb_checksum_setup(skb)) { >> - kfree_skb(skb); >> - packets_dropped++; >> - dev->stats.rx_errors++; >> - continue; >> - } >> + if (skb->ip_summed != CHECKSUM_PARTIAL || >> + skb_checksum_setup(skb)) { > That drops non-partial skbs. However they are fine unless they also > claim to be gso. > > Perhaps you meant "skb->ip_summed == CHECKSUM_PARTIAL && ! > skb_checksum_setup(skb)" which I think works but doesn't allow us to > correctly chain the gso check onto the else. No, I didn't mean to drop the skb_is_gso() test. But still, the if()s can be folded to share the same body. J ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-24 17:55 ` Jeremy Fitzhardinge @ 2011-01-25 17:09 ` Ian Campbell 2011-01-25 17:09 ` Ian Campbell 2011-01-25 17:10 ` Ian Campbell 0 siblings, 2 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-25 17:09 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: netdev, xen-devel On Mon, 2011-01-24 at 17:55 +0000, Jeremy Fitzhardinge wrote: > No, I didn't mean to drop the skb_is_gso() test. But still, the if()s > can be folded to share the same body. My original opinion was that sharing that bit of body at the expense of an increasingly hard to decipher if conditional was not a good trade off. However thinking about it again I think the test logic would be better of factored out into a validate_incoming_skb() type function anyway. Short (2 patch) replacement series follows. Ian. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] xen: netfront: refactor code for checking validity of incoming skbs 2011-01-25 17:09 ` Ian Campbell @ 2011-01-25 17:09 ` Ian Campbell 2011-01-25 17:10 ` Ian Campbell 1 sibling, 0 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-25 17:09 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: Ian Campbell, Jeremy Fitzhardinge, xen-devel, netdev Makes future additional validation clearer. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: xen-devel@lists.xensource.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 24 +++++++++++++++++------- 1 files changed, 17 insertions(+), 7 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 546de57..4dc347b 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -809,6 +809,18 @@ out: return err; } +static int validate_incoming_skb(struct sk_buff *skb) +{ + /* + * If the SKB is partial then we must be able to setup the + * checksum fields in the skb. + */ + if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_setup(skb)) + return 0; + + return 1; +} + static int handle_incoming_queue(struct net_device *dev, struct sk_buff_head *rxq) { @@ -829,13 +841,11 @@ static int handle_incoming_queue(struct net_device *dev, /* Ethernet work: Delayed to here as it peeks the header. */ skb->protocol = eth_type_trans(skb, dev); - if (skb->ip_summed == CHECKSUM_PARTIAL) { - if (skb_checksum_setup(skb)) { - kfree_skb(skb); - packets_dropped++; - dev->stats.rx_errors++; - continue; - } + if (!validate_incoming_skb(skb)) { + kfree_skb(skb); + packets_dropped++; + dev->stats.rx_errors++; + continue; } dev->stats.rx_packets++; -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 1/2] xen: netfront: refactor code for checking validity of incoming skbs @ 2011-01-25 17:09 ` Ian Campbell 0 siblings, 0 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-25 17:09 UTC (permalink / raw) Cc: Ian Campbell, Jeremy Fitzhardinge, xen-devel, netdev Makes future additional validation clearer. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: xen-devel@lists.xensource.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 24 +++++++++++++++++------- 1 files changed, 17 insertions(+), 7 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 546de57..4dc347b 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -809,6 +809,18 @@ out: return err; } +static int validate_incoming_skb(struct sk_buff *skb) +{ + /* + * If the SKB is partial then we must be able to setup the + * checksum fields in the skb. + */ + if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_setup(skb)) + return 0; + + return 1; +} + static int handle_incoming_queue(struct net_device *dev, struct sk_buff_head *rxq) { @@ -829,13 +841,11 @@ static int handle_incoming_queue(struct net_device *dev, /* Ethernet work: Delayed to here as it peeks the header. */ skb->protocol = eth_type_trans(skb, dev); - if (skb->ip_summed == CHECKSUM_PARTIAL) { - if (skb_checksum_setup(skb)) { - kfree_skb(skb); - packets_dropped++; - dev->stats.rx_errors++; - continue; - } + if (!validate_incoming_skb(skb)) { + kfree_skb(skb); + packets_dropped++; + dev->stats.rx_errors++; + continue; } dev->stats.rx_packets++; -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-25 17:09 ` Ian Campbell @ 2011-01-25 17:10 ` Ian Campbell 2011-01-25 17:10 ` Ian Campbell 1 sibling, 0 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-25 17:10 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: Ian Campbell, Jeremy Fitzhardinge, xen-devel, netdev The Linux network stack expects all GSO SKBs to have ip_summed == CHECKSUM_PARTIAL (which implies that the frame contains a partial checksum) and the Xen network ring protocol similarly expects an SKB which has GSO set to also have NETRX_csum_blank (which also implies a partial checksum). Therefore drop such frames on receive otherwise they will trigger the warning in skb_gso_segment. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: xen-devel@lists.xensource.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 4dc347b..0ea47da 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -818,6 +818,10 @@ static int validate_incoming_skb(struct sk_buff *skb) if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_setup(skb)) return 0; + /* A GSO SKB must be partial. */ + if (skb->ip_summed != CHECKSUM_PARTIAL && skb_is_gso(skb)) + return 0; + return 1; } -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] xen: netfront: Drop GSO SKBs which do not have csum_blank. @ 2011-01-25 17:10 ` Ian Campbell 0 siblings, 0 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-25 17:10 UTC (permalink / raw) Cc: Ian Campbell, Jeremy Fitzhardinge, xen-devel, netdev The Linux network stack expects all GSO SKBs to have ip_summed == CHECKSUM_PARTIAL (which implies that the frame contains a partial checksum) and the Xen network ring protocol similarly expects an SKB which has GSO set to also have NETRX_csum_blank (which also implies a partial checksum). Therefore drop such frames on receive otherwise they will trigger the warning in skb_gso_segment. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: xen-devel@lists.xensource.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 4dc347b..0ea47da 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -818,6 +818,10 @@ static int validate_incoming_skb(struct sk_buff *skb) if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_setup(skb)) return 0; + /* A GSO SKB must be partial. */ + if (skb->ip_summed != CHECKSUM_PARTIAL && skb_is_gso(skb)) + return 0; + return 1; } -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-25 17:10 ` Ian Campbell (?) @ 2011-01-26 3:44 ` David Miller 2011-01-26 11:56 ` Ian Campbell -1 siblings, 1 reply; 15+ messages in thread From: David Miller @ 2011-01-26 3:44 UTC (permalink / raw) To: ian.campbell; +Cc: netdev, jeremy, xen-devel From: Ian Campbell <ian.campbell@citrix.com> Date: Tue, 25 Jan 2011 17:10:00 +0000 > The Linux network stack expects all GSO SKBs to have ip_summed == > CHECKSUM_PARTIAL (which implies that the frame contains a partial > checksum) and the Xen network ring protocol similarly expects an SKB > which has GSO set to also have NETRX_csum_blank (which also implies a > partial checksum). Therefore drop such frames on receive otherwise > they will trigger the warning in skb_gso_segment. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> The GSO code does in fact warn in the logs about this situation, but it _DOES NOT_ drop the packet. Therefore, either you guys should do the same or we should make the generic code drop too. I think the generic code is doing the right thing, therefore what you should probably do is put the checksum of the SKB into the right state when you detect this situation (and perhaps bump a ethtool driver local statistic which specifically tracks this exact event). Or, even better, you should fix whatever causes this in the first place. Dropping frames ought to be the last option, stuff like this is impossible to debug if someone starts wondering why they are getting frame drops. You don't even account for this in a unique statistic somewhere, so people can figure out the actual spcific _reason_ for the drop. They will just see "rx_error" and scratch their heads. Anyways, I think dropping is fundamentally wrong, so I'm not applying this. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xen: netfront: Drop GSO SKBs which do not have csum_blank. 2011-01-26 3:44 ` David Miller @ 2011-01-26 11:56 ` Ian Campbell 2011-01-27 14:14 ` Ian Campbell 0 siblings, 1 reply; 15+ messages in thread From: Ian Campbell @ 2011-01-26 11:56 UTC (permalink / raw) To: David Miller; +Cc: jeremy, xen-devel, netdev On Wed, 2011-01-26 at 03:44 +0000, David Miller wrote: > From: Ian Campbell <ian.campbell@citrix.com> > Date: Tue, 25 Jan 2011 17:10:00 +0000 > > > The Linux network stack expects all GSO SKBs to have ip_summed == > > CHECKSUM_PARTIAL (which implies that the frame contains a partial > > checksum) and the Xen network ring protocol similarly expects an SKB > > which has GSO set to also have NETRX_csum_blank (which also implies a > > partial checksum). Therefore drop such frames on receive otherwise > > they will trigger the warning in skb_gso_segment. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > The GSO code does in fact warn in the logs about this situation, but > it _DOES NOT_ drop the packet. Therefore, either you guys should do > the same or we should make the generic code drop too. Ah, yes. I misread the handling of an error from pskb_expand_head() in skb_gso_segment() and thought it was a more general error return covering the entire case. > I think the generic code is doing the right thing, therefore what you > should probably do is put the checksum of the SKB into the right state > when you detect this situation (and perhaps bump a ethtool driver > local statistic which specifically tracks this exact event). Yes, I think this is a good idea. I'll come up with a patch which does this. > Or, even better, you should fix whatever causes this in the first > place. Sure, that has already been done but the proper fix is in another guest, with a secondary robustness fix in netback (similar to this one, so I'll take your advice from above on board in that context too). The intention here was to be robust in the face of unfixed guests sharing the same host or future netback bugs etc. > Dropping frames ought to be the last option, stuff like this is > impossible to debug if someone starts wondering why they are getting > frame drops. > > You don't even account for this in a unique statistic somewhere, so > people can figure out the actual spcific _reason_ for the drop. They > will just see "rx_error" and scratch their heads. > > Anyways, I think dropping is fundamentally wrong, so I'm not applying > this. You've convinced me too, thanks for the feedback. Thanks, Ian. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] xen: netfront: handle incoming GSO SKBs which are not CHECKSUM_PARTIAL 2011-01-26 11:56 ` Ian Campbell @ 2011-01-27 14:14 ` Ian Campbell 0 siblings, 0 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-27 14:14 UTC (permalink / raw) To: netdev; +Cc: Ian Campbell, Jeremy Fitzhardinge, David Miller, xen-devel, netdev The Linux network stack expects all GSO SKBs to have ip_summed == CHECKSUM_PARTIAL (which implies that the frame contains a partial checksum) and the Xen network ring protocol similarly expects an SKB which has GSO set to also have NETRX_csum_blank (which also implies a partial checksum). However there have been cases of buggy guests which mark a frame as GSO but do not set csum_blank. If we detect that we a receiving such a frame (which manifests as ip_summed != PARTIAL && skb_is_gso) then force the SKB to partial and recalculate the checksum, since we cannot rely on the peer having done so if they have not set csum_blank. Add an ethtool stat to track occurances of this event. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: David Miller <davem@davemloft.net> Cc: xen-devel@lists.xensource.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 96 ++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 88 insertions(+), 8 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 546de57..da1f121 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -120,6 +120,9 @@ struct netfront_info { unsigned long rx_pfn_array[NET_RX_RING_SIZE]; struct multicall_entry rx_mcl[NET_RX_RING_SIZE+1]; struct mmu_update rx_mmu[NET_RX_RING_SIZE]; + + /* Statistics */ + int rx_gso_checksum_fixup; }; struct netfront_rx_info { @@ -770,11 +773,29 @@ static RING_IDX xennet_fill_frags(struct netfront_info *np, return cons; } -static int skb_checksum_setup(struct sk_buff *skb) +static int checksum_setup(struct net_device *dev, struct sk_buff *skb) { struct iphdr *iph; unsigned char *th; int err = -EPROTO; + int recalculate_partial_csum = 0; + + /* + * A GSO SKB must be CHECKSUM_PARTIAL. However some buggy + * peers can fail to set NETRXF_csum_blank when sending a GSO + * frame. In this case force the SKB to CHECKSUM_PARTIAL and + * recalculate the partial checksum. + */ + if (skb->ip_summed != CHECKSUM_PARTIAL && skb_is_gso(skb)) { + struct netfront_info *np = netdev_priv(dev); + np->rx_gso_checksum_fixup++; + skb->ip_summed = CHECKSUM_PARTIAL; + recalculate_partial_csum = 1; + } + + /* A non-CHECKSUM_PARTIAL SKB does not require setup. */ + if (skb->ip_summed != CHECKSUM_PARTIAL) + return 0; if (skb->protocol != htons(ETH_P_IP)) goto out; @@ -788,9 +809,23 @@ static int skb_checksum_setup(struct sk_buff *skb) switch (iph->protocol) { case IPPROTO_TCP: skb->csum_offset = offsetof(struct tcphdr, check); + + if (recalculate_partial_csum) { + struct tcphdr *tcph = (struct tcphdr *)th; + tcph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, + skb->len - iph->ihl*4, + IPPROTO_TCP, 0); + } break; case IPPROTO_UDP: skb->csum_offset = offsetof(struct udphdr, check); + + if (recalculate_partial_csum) { + struct udphdr *udph = (struct udphdr *)th; + udph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, + skb->len - iph->ihl*4, + IPPROTO_UDP, 0); + } break; default: if (net_ratelimit()) @@ -829,13 +864,11 @@ static int handle_incoming_queue(struct net_device *dev, /* Ethernet work: Delayed to here as it peeks the header. */ skb->protocol = eth_type_trans(skb, dev); - if (skb->ip_summed == CHECKSUM_PARTIAL) { - if (skb_checksum_setup(skb)) { - kfree_skb(skb); - packets_dropped++; - dev->stats.rx_errors++; - continue; - } + if (checksum_setup(dev, skb)) { + kfree_skb(skb); + packets_dropped++; + dev->stats.rx_errors++; + continue; } dev->stats.rx_packets++; @@ -1632,12 +1665,59 @@ static void netback_changed(struct xenbus_device *dev, } } +static const struct xennet_stat { + char name[ETH_GSTRING_LEN]; + u16 offset; +} xennet_stats[] = { + { + "rx_gso_checksum_fixup", + offsetof(struct netfront_info, rx_gso_checksum_fixup) + }, +}; + +static int xennet_get_sset_count(struct net_device *dev, int string_set) +{ + switch (string_set) { + case ETH_SS_STATS: + return ARRAY_SIZE(xennet_stats); + default: + return -EINVAL; + } +} + +static void xennet_get_ethtool_stats(struct net_device *dev, + struct ethtool_stats *stats, u64 * data) +{ + void *np = netdev_priv(dev); + int i; + + for (i = 0; i < ARRAY_SIZE(xennet_stats); i++) + data[i] = *(int *)(np + xennet_stats[i].offset); +} + +static void xennet_get_strings(struct net_device *dev, u32 stringset, u8 * data) +{ + int i; + + switch (stringset) { + case ETH_SS_STATS: + for (i = 0; i < ARRAY_SIZE(xennet_stats); i++) + memcpy(data + i * ETH_GSTRING_LEN, + xennet_stats[i].name, ETH_GSTRING_LEN); + break; + } +} + static const struct ethtool_ops xennet_ethtool_ops = { .set_tx_csum = ethtool_op_set_tx_csum, .set_sg = xennet_set_sg, .set_tso = xennet_set_tso, .get_link = ethtool_op_get_link, + + .get_sset_count = xennet_get_sset_count, + .get_ethtool_stats = xennet_get_ethtool_stats, + .get_strings = xennet_get_strings, }; #ifdef CONFIG_SYSFS -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] xen: netfront: handle incoming GSO SKBs which are not CHECKSUM_PARTIAL @ 2011-01-27 14:14 ` Ian Campbell 0 siblings, 0 replies; 15+ messages in thread From: Ian Campbell @ 2011-01-27 14:14 UTC (permalink / raw) Cc: Ian Campbell, Jeremy Fitzhardinge, David Miller, xen-devel, netdev The Linux network stack expects all GSO SKBs to have ip_summed == CHECKSUM_PARTIAL (which implies that the frame contains a partial checksum) and the Xen network ring protocol similarly expects an SKB which has GSO set to also have NETRX_csum_blank (which also implies a partial checksum). However there have been cases of buggy guests which mark a frame as GSO but do not set csum_blank. If we detect that we a receiving such a frame (which manifests as ip_summed != PARTIAL && skb_is_gso) then force the SKB to partial and recalculate the checksum, since we cannot rely on the peer having done so if they have not set csum_blank. Add an ethtool stat to track occurances of this event. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Cc: David Miller <davem@davemloft.net> Cc: xen-devel@lists.xensource.com Cc: netdev@vger.kernel.org --- drivers/net/xen-netfront.c | 96 ++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 88 insertions(+), 8 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 546de57..da1f121 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -120,6 +120,9 @@ struct netfront_info { unsigned long rx_pfn_array[NET_RX_RING_SIZE]; struct multicall_entry rx_mcl[NET_RX_RING_SIZE+1]; struct mmu_update rx_mmu[NET_RX_RING_SIZE]; + + /* Statistics */ + int rx_gso_checksum_fixup; }; struct netfront_rx_info { @@ -770,11 +773,29 @@ static RING_IDX xennet_fill_frags(struct netfront_info *np, return cons; } -static int skb_checksum_setup(struct sk_buff *skb) +static int checksum_setup(struct net_device *dev, struct sk_buff *skb) { struct iphdr *iph; unsigned char *th; int err = -EPROTO; + int recalculate_partial_csum = 0; + + /* + * A GSO SKB must be CHECKSUM_PARTIAL. However some buggy + * peers can fail to set NETRXF_csum_blank when sending a GSO + * frame. In this case force the SKB to CHECKSUM_PARTIAL and + * recalculate the partial checksum. + */ + if (skb->ip_summed != CHECKSUM_PARTIAL && skb_is_gso(skb)) { + struct netfront_info *np = netdev_priv(dev); + np->rx_gso_checksum_fixup++; + skb->ip_summed = CHECKSUM_PARTIAL; + recalculate_partial_csum = 1; + } + + /* A non-CHECKSUM_PARTIAL SKB does not require setup. */ + if (skb->ip_summed != CHECKSUM_PARTIAL) + return 0; if (skb->protocol != htons(ETH_P_IP)) goto out; @@ -788,9 +809,23 @@ static int skb_checksum_setup(struct sk_buff *skb) switch (iph->protocol) { case IPPROTO_TCP: skb->csum_offset = offsetof(struct tcphdr, check); + + if (recalculate_partial_csum) { + struct tcphdr *tcph = (struct tcphdr *)th; + tcph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, + skb->len - iph->ihl*4, + IPPROTO_TCP, 0); + } break; case IPPROTO_UDP: skb->csum_offset = offsetof(struct udphdr, check); + + if (recalculate_partial_csum) { + struct udphdr *udph = (struct udphdr *)th; + udph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, + skb->len - iph->ihl*4, + IPPROTO_UDP, 0); + } break; default: if (net_ratelimit()) @@ -829,13 +864,11 @@ static int handle_incoming_queue(struct net_device *dev, /* Ethernet work: Delayed to here as it peeks the header. */ skb->protocol = eth_type_trans(skb, dev); - if (skb->ip_summed == CHECKSUM_PARTIAL) { - if (skb_checksum_setup(skb)) { - kfree_skb(skb); - packets_dropped++; - dev->stats.rx_errors++; - continue; - } + if (checksum_setup(dev, skb)) { + kfree_skb(skb); + packets_dropped++; + dev->stats.rx_errors++; + continue; } dev->stats.rx_packets++; @@ -1632,12 +1665,59 @@ static void netback_changed(struct xenbus_device *dev, } } +static const struct xennet_stat { + char name[ETH_GSTRING_LEN]; + u16 offset; +} xennet_stats[] = { + { + "rx_gso_checksum_fixup", + offsetof(struct netfront_info, rx_gso_checksum_fixup) + }, +}; + +static int xennet_get_sset_count(struct net_device *dev, int string_set) +{ + switch (string_set) { + case ETH_SS_STATS: + return ARRAY_SIZE(xennet_stats); + default: + return -EINVAL; + } +} + +static void xennet_get_ethtool_stats(struct net_device *dev, + struct ethtool_stats *stats, u64 * data) +{ + void *np = netdev_priv(dev); + int i; + + for (i = 0; i < ARRAY_SIZE(xennet_stats); i++) + data[i] = *(int *)(np + xennet_stats[i].offset); +} + +static void xennet_get_strings(struct net_device *dev, u32 stringset, u8 * data) +{ + int i; + + switch (stringset) { + case ETH_SS_STATS: + for (i = 0; i < ARRAY_SIZE(xennet_stats); i++) + memcpy(data + i * ETH_GSTRING_LEN, + xennet_stats[i].name, ETH_GSTRING_LEN); + break; + } +} + static const struct ethtool_ops xennet_ethtool_ops = { .set_tx_csum = ethtool_op_set_tx_csum, .set_sg = xennet_set_sg, .set_tso = xennet_set_tso, .get_link = ethtool_op_get_link, + + .get_sset_count = xennet_get_sset_count, + .get_ethtool_stats = xennet_get_ethtool_stats, + .get_strings = xennet_get_strings, }; #ifdef CONFIG_SYSFS -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] xen: netfront: handle incoming GSO SKBs which are not CHECKSUM_PARTIAL 2011-01-27 14:14 ` Ian Campbell (?) @ 2011-01-27 22:23 ` David Miller -1 siblings, 0 replies; 15+ messages in thread From: David Miller @ 2011-01-27 22:23 UTC (permalink / raw) To: ian.campbell; +Cc: netdev, jeremy, xen-devel From: Ian Campbell <ian.campbell@citrix.com> Date: Thu, 27 Jan 2011 14:14:03 +0000 > The Linux network stack expects all GSO SKBs to have ip_summed == > CHECKSUM_PARTIAL (which implies that the frame contains a partial > checksum) and the Xen network ring protocol similarly expects an SKB > which has GSO set to also have NETRX_csum_blank (which also implies a > partial checksum). > > However there have been cases of buggy guests which mark a frame as > GSO but do not set csum_blank. If we detect that we a receiving such a > frame (which manifests as ip_summed != PARTIAL && skb_is_gso) then > force the SKB to partial and recalculate the checksum, since we cannot > rely on the peer having done so if they have not set csum_blank. > > Add an ethtool stat to track occurances of this event. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Looks great, applied, thanks Ian. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-01-27 22:23 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-05 13:23 [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank Ian Campbell 2011-01-11 11:46 ` Ian Campbell 2011-01-22 0:58 ` Jeremy Fitzhardinge 2011-01-22 9:43 ` Ian Campbell 2011-01-24 17:55 ` Jeremy Fitzhardinge 2011-01-25 17:09 ` Ian Campbell 2011-01-25 17:09 ` [PATCH 1/2] xen: netfront: refactor code for checking validity of incoming skbs Ian Campbell 2011-01-25 17:09 ` Ian Campbell 2011-01-25 17:10 ` [PATCH 2/2] xen: netfront: Drop GSO SKBs which do not have csum_blank Ian Campbell 2011-01-25 17:10 ` Ian Campbell 2011-01-26 3:44 ` David Miller 2011-01-26 11:56 ` Ian Campbell 2011-01-27 14:14 ` [PATCH] xen: netfront: handle incoming GSO SKBs which are not CHECKSUM_PARTIAL Ian Campbell 2011-01-27 14:14 ` Ian Campbell 2011-01-27 22:23 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.