[PATCHv2-net-3.8 0/3] fix kernel crash with macvtap on top of LRO

[PATCHv2-net-3.8 0/3] fix kernel crash with macvtap on top of LRO

[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.

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(-)

-- 
MST

[PATCHv2-net-3.8 1/3] ixgbe: fix gso type

[Michael S. Tsirkin]

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

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 20a5af6..e1b2d22 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -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,
-- 
MST

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

[Michael S. Tsirkin]

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

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
index 6f82812..09aa310 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
@@ -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);
-- 
MST

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

[Jitendra Kalsaria]

On 2/7/13 5:13 AM, "Michael S. Tsirkin" <mst@redhat.com> wrote:

>qlcnic set gso_size but not gso type. This leads to crashes
>in macvtap.
>
>Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>---
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
>b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
>index 6f82812..09aa310 100644
>--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
>+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
>@@ -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);
>-- 
>MST

Thanks!

Acked-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

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

[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 <mst@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index f771ddf..a5edac8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -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;
 	}
 
 
-- 
MST

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

[Dmitry Kravkov]

On Thu, 2013-02-07 at 15:13 +0200, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index f771ddf..a5edac8 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -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;
>  	}
>  
> 
Thanks!

Acked-by: Dmitry Kravkov <dmitry@broadcom.com>

Re: [PATCHv2-net-3.8 0/3] fix kernel crash with macvtap on top of LRO

[Ben Hutchings]

On Thu, 2013-02-07 at 15:12 +0200, Michael S. Tsirkin wrote:
> 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

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.

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

Re: [PATCHv2-net-3.8 0/3] fix kernel crash with macvtap on top of LRO

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 7 Feb 2013 15:12:56 +0200

> 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

Applied to 'net', thanks.