| Message ID | 4D3A2BD2.5030802@goop.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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++;