fix kernel crash with macvtap on top of LRO

PATCH Applied Michael S. Tsirkin 5 participants 8 messages Feb 7, 2013 – Feb 11, 2013

Subsystems (5)

Networking Network drivers Intel Ethernet Broadcom BNX2X 10 gigabit Ethernet driver +1 more subsystems

lore

Share & tools

lore

Michael S. Tsirkin

author

2013-02-07 13:08 UTC #0 lore raw

cover fix kernel crash with macvtap on top of LRO

At the moment, macvtap crashes are observed if macvtap is attached to an interface with LRO enabled.
The crash in question is BUG() in macvtap_skb_to_vnet_hdr. This happens because several drivers set gso_size but not gso_type in incoming skbs.
This didn't use to be the case: with intel cards on 3.2 and older kernels, with qlogic - on 3.4 and older kernels, so it's a regression if not a recent one.
The following patches fix this for qlogic, broadcom and intel drivers.

I tested that the patch fixes the crash for ixgbe but don't have qlogic/broadcom hardware to test. I also only tested TCPv4.

Please review, and consider for 3.8.

Changes from v1:
- added missing htons as suggested by Eric
- backported the relevant bits from cbf1de72324a8105ddcc3d9ce9acbc613faea17e for bnx2x

Michael S. Tsirkin (3):
ixgbe: fix gso type
qlcnic: set gso_type
bnx2x: set gso_type

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 12 +++++-------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   |  8 ++++++--
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c  |  7 ++++++-
 3 files changed, 17 insertions(+), 10 deletions(-)

Signature · 1 line

MST

Michael S. Tsirkin

author

2013-02-07 13:09 UTC #1 lore raw

PATCH 1/3 ixgbe: fix gso type

ixgbe set gso_size but not gso_type. This leads to crashes in macvtap.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +6 -2

index 20a5af6..e1b2d22 100644
@@ -1401,6 +1401,10 @@ static void ixgbe_set_rsc_gso_size(struct ixgbe_ring *ring,
 	/* set gso_size to avoid messing up TCP MSS */
 	skb_shinfo(skb)->gso_size = DIV_ROUND_UP((skb->len - hdr_len),
 						 IXGBE_CB(skb)->append_cnt);
+	if (skb->protocol == __constant_htons(ETH_P_IPV6))
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+	else
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
 }
 
 static void ixgbe_update_rsc_stats(struct ixgbe_ring *rx_ring,

@@ -1435,6 +1439,8 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
 {
 	struct net_device *dev = rx_ring->netdev;
 
+	skb->protocol = eth_type_trans(skb, dev);
+
 	ixgbe_update_rsc_stats(rx_ring, skb);
 
 	ixgbe_rx_hash(rx_ring, rx_desc, skb);

@@ -1450,8 +1456,6 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
 	}
 
 	skb_record_rx_queue(skb, rx_ring->queue_index);
-
-	skb->protocol = eth_type_trans(skb, dev);
 }
 
 static void ixgbe_rx_skb(struct ixgbe_q_vector *q_vector,

Signature · 1 line

MST

Michael S. Tsirkin

author

2013-02-07 13:09 UTC #2 lore raw

PATCH 2/3 qlcnic: set gso_type

In reply to Michael S. Tsirkin

qlcnic set gso_size but not gso type. This leads to crashes in macvtap.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +6 -1

index 6f82812..09aa310 100644
@@ -986,8 +986,13 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 	th->seq = htonl(seq_number);
 	length = skb->len;
 
-	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP)
+	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP) {
 		skb_shinfo(skb)->gso_size = qlcnic_get_lro_sts_mss(sts_data1);
+		if (skb->protocol == htons(ETH_P_IPV6))
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+		else
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	}
 
 	if (vid != 0xffff)
 		__vlan_hwaccel_put_tag(skb, vid);

Signature · 1 line

MST

Jitendra Kalsaria

Acked-by on v1 2/3

2013-02-07 17:08 UTC #3 lore raw

Re: [PATCHv2-net-3.8 2/3] qlcnic: set gso_type

Quoted 30 lines · Michael S. Tsirkin

qlcnic set gso_size but not gso type. This leads to crashes in macvtap.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-

1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +6 -1

index 6f82812..09aa310 100644
@@ -986,8 +986,13 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 	th->seq = htonl(seq_number);
 	length = skb->len;
 
-	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP)
+	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP) {
 		skb_shinfo(skb)->gso_size = qlcnic_get_lro_sts_mss(sts_data1);
+		if (skb->protocol == htons(ETH_P_IPV6))
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+		else
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	}
 
 	if (vid != 0xffff)
 		__vlan_hwaccel_put_tag(skb, vid);

Signature · 1 line

MST

Thanks!

Acked-by Jitendra Kalsaria <[email protected]>

Michael S. Tsirkin

author

2013-02-07 13:09 UTC #4 lore raw

PATCH 3/3 bnx2x: set gso_type

In reply to Michael S. Tsirkin

In LRO mode, bnx2x set gso_size but not gso type. This leads to crashes in macvtap.
Commit cbf1de72324a8105ddcc3d9ce9acbc613faea17e queued for 3.9 includes a more complete fix. This is a minimal patch to avoid the crash, for 3.8.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5 -7

index f771ddf..a5edac8 100644
@@ -504,13 +504,11 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 		skb_shinfo(skb)->gso_size = bnx2x_set_lro_mss(bp,
 					tpa_info->parsing_flags, len_on_bd);
 
-		/* set for GRO */
-		if (fp->mode == TPA_MODE_GRO)
-			skb_shinfo(skb)->gso_type =
-			    (GET_FLAG(tpa_info->parsing_flags,
-				      PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
-						PRS_FLAG_OVERETH_IPV6) ?
-				SKB_GSO_TCPV6 : SKB_GSO_TCPV4;
+		skb_shinfo(skb)->gso_type =
+			(GET_FLAG(tpa_info->parsing_flags,
+				  PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
+			 PRS_FLAG_OVERETH_IPV6) ?
+			SKB_GSO_TCPV6 : SKB_GSO_TCPV4;
 	}

Signature · 1 line

MST

Dmitry Kravkov

Acked-by on v1 3/3

2013-02-07 13:37 UTC #5 lore raw

Re: [PATCHv2-net-3.8 3/3] bnx2x: set gso_type

Quoted 24 lines · Michael S. Tsirkin

In LRO mode, bnx2x set gso_size but not gso type. This leads to crashes in macvtap.
Commit cbf1de72324a8105ddcc3d9ce9acbc613faea17e queued for 3.9 includes a more complete fix. This is a minimal patch to avoid the crash, for 3.8.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0 -5
index f771ddf..a5edac8 100644
@@ -504,13 +504,11 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 		skb_shinfo(skb)->gso_size = bnx2x_set_lro_mss(bp,
 					tpa_info->parsing_flags, len_on_bd);
 
-		/* set for GRO */
-		if (fp->mode == TPA_MODE_GRO)
-			skb_shinfo(skb)->gso_type =
-			    (GET_FLAG(tpa_info->parsing_flags,
-				      PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp, · line 514

Quoted 10 lines

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5 -2

-						PRS_FLAG_OVERETH_IPV6) ?
-				SKB_GSO_TCPV6 : SKB_GSO_TCPV4;
+		skb_shinfo(skb)->gso_type =
+			(GET_FLAG(tpa_info->parsing_flags,
+				  PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
+			 PRS_FLAG_OVERETH_IPV6) ?
+			SKB_GSO_TCPV6 : SKB_GSO_TCPV4;
 	}

Thanks!

Acked-by Dmitry Kravkov <[email protected]>

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Ben Hutchings

2013-02-07 17:05 UTC #6 lore raw

In reply to Michael S. Tsirkin

Quoted 14 lines · Michael S. Tsirkin

At the moment, macvtap crashes are observed if macvtap is attached to an interface with LRO enabled.
The crash in question is BUG() in macvtap_skb_to_vnet_hdr. This happens because several drivers set gso_size but not gso_type in incoming skbs.
This didn't use to be the case: with intel cards on 3.2 and older kernels, with qlogic - on 3.4 and older kernels, so it's a regression if not a recent one.
The following patches fix this for qlogic, broadcom and intel drivers.
I tested that the patch fixes the crash for ixgbe but don't have qlogic/broadcom hardware to test. I also only tested TCPv4.

Quoted 6 lines

Please review, and consider for 3.8.
Changes from v1:
- added missing htons as suggested by Eric
- backported the relevant bits from cbf1de72324a8105ddcc3d9ce9acbc613faea17e for bnx2x

I still don't think macvtap should behave inconsistently with forwarding and the bridge device. We need a consensus as to whether transformation by LRO is allowable before making changes one way or the other. If we're going to allow it then inet_lro also needs this fix.

Ben.

Signature · 15 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

David Miller

maintainer Applied

2013-02-11 01:15 UTC #7 lore raw

In reply to Michael S. Tsirkin

From: "Michael S. Tsirkin" <[email protected]>
Date: Thu, 7 Feb 2013 15:12:56 +0200

Quoted 14 lines

At the moment, macvtap crashes are observed if macvtap is attached to an interface with LRO enabled.
The crash in question is BUG() in macvtap_skb_to_vnet_hdr. This happens because several drivers set gso_size but not gso_type in incoming skbs.
This didn't use to be the case: with intel cards on 3.2 and older kernels, with qlogic - on 3.4 and older kernels, so it's a regression if not a recent one.
The following patches fix this for qlogic, broadcom and intel drivers.
I tested that the patch fixes the crash for ixgbe but don't have qlogic/broadcom hardware to test. I also only tested TCPv4.

Quoted 6 lines

Please review, and consider for 3.8.
Changes from v1:
- added missing htons as suggested by Eric
- backported the relevant bits from cbf1de72324a8105ddcc3d9ce9acbc613faea17e for bnx2x

Applied to 'net', thanks.