[PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank.

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 <redacted>
Cc: Jeremy Fitzhardinge <redacted>
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

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:

On 01/05/2011 05:23 AM, Ian Campbell wrote:

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

On Sat, 2011-01-22 at 00:58 +0000, Jeremy Fitzhardinge wrote: 

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

On 01/22/2011 01:43 AM, Ian Campbell wrote:

On Sat, 2011-01-22 at 00:58 +0000, Jeremy Fitzhardinge wrote: 

quoted

On 01/05/2011 05:23 AM, Ian Campbell wrote:

quoted

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 <redacted>
Cc: Jeremy Fitzhardinge <redacted>
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

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.

Makes future additional validation clearer.

Signed-off-by: Ian Campbell <redacted>
Cc: Jeremy Fitzhardinge <redacted>
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

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 <redacted>
Cc: Jeremy Fitzhardinge <redacted>
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

From: Ian Campbell <redacted>
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 <redacted>

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.

On Wed, 2011-01-26 at 03:44 +0000, David Miller wrote:

From: Ian Campbell <redacted>
Date: Tue, 25 Jan 2011 17:10:00 +0000

quoted

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 <redacted>

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.

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 <redacted>
Cc: Jeremy Fitzhardinge <redacted>
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

From: Ian Campbell <redacted>
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 <redacted>

Looks great, applied, thanks Ian.