Patchwork [2/2] be2net: drop non-tso frames longer than mtu

login
register
mail settings
Submitter Sathya Perla
Date Oct. 15, 2013, 11:56 a.m.
Message ID <1381838188-9625-2-git-send-email-sathya.perla@emulex.com>
Download mbox | patch
Permalink /patch/283596/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Sathya Perla - Oct. 15, 2013, 11:56 a.m.
From: Vasundhara Volam <vasundhara.volam@emulex.com>

Pktgen can generate non-TSO frames of arbitrary length disregarding
the MTU value of the physical interface. Drop such frames in the driver
instead of sending them to HW as it cannot handle such frames.

Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
 drivers/net/ethernet/emulex/benet/be_main.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Sergei Shtylyov - Oct. 15, 2013, 1:45 p.m.
Hello.

On 15-10-2013 15:56, Sathya Perla wrote:

> From: Vasundhara Volam <vasundhara.volam@emulex.com>

> Pktgen can generate non-TSO frames of arbitrary length disregarding
> the MTU value of the physical interface. Drop such frames in the driver
> instead of sending them to HW as it cannot handle such frames.

> Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
> ---
>   drivers/net/ethernet/emulex/benet/be_main.c |    9 +++++++--
>   1 files changed, 7 insertions(+), 2 deletions(-)

> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index 2c38cc4..76057b8 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter,
>   	unsigned int eth_hdr_len;
>   	struct iphdr *ip;
>
> +	/* Don't allow non-TSO packets longer than MTU */
> +	eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ?
> +			VLAN_ETH_HLEN : ETH_HLEN;
> +	if (!skb_is_gso(skb) &&
> +	    (skb->len - eth_hdr_len) > adapter->netdev->mtu)
> +			goto tx_drop;

    This *goto* is indented one tab too much.

WBR, Sergei

--
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
Eric Dumazet - Oct. 15, 2013, 1:47 p.m.
On Tue, 2013-10-15 at 17:26 +0530, Sathya Perla wrote:
> From: Vasundhara Volam <vasundhara.volam@emulex.com>
> 
> Pktgen can generate non-TSO frames of arbitrary length disregarding
> the MTU value of the physical interface. Drop such frames in the driver
> instead of sending them to HW as it cannot handle such frames.
> 
> Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
> ---
>  drivers/net/ethernet/emulex/benet/be_main.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index 2c38cc4..76057b8 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter,
>  	unsigned int eth_hdr_len;
>  	struct iphdr *ip;
>  
> +	/* Don't allow non-TSO packets longer than MTU */
> +	eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ?
> +			VLAN_ETH_HLEN : ETH_HLEN;
> +	if (!skb_is_gso(skb) &&
> +	    (skb->len - eth_hdr_len) > adapter->netdev->mtu)
> +			goto tx_drop;
> +

When you say 'cannot handle them', is it some kind of nasty thing like
hang / crash ?

One could imagine gso_size + sizeof(headers) > mtu, and give the same
problem ?


AFAIK, we have no check in net/core/dev.c.

Maybe we should instead add them there (and in pktgen)



--
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
Ivan Vecera - Oct. 15, 2013, 2:30 p.m.
On 10/15/2013 03:47 PM, Eric Dumazet wrote:
> On Tue, 2013-10-15 at 17:26 +0530, Sathya Perla wrote:
>> From: Vasundhara Volam <vasundhara.volam@emulex.com>
>>
>> Pktgen can generate non-TSO frames of arbitrary length disregarding
>> the MTU value of the physical interface. Drop such frames in the driver
>> instead of sending them to HW as it cannot handle such frames.
>>
>> Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
>> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
>> ---
>>   drivers/net/ethernet/emulex/benet/be_main.c |    9 +++++++--
>>   1 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
>> index 2c38cc4..76057b8 100644
>> --- a/drivers/net/ethernet/emulex/benet/be_main.c
>> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
>> @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter,
>>   	unsigned int eth_hdr_len;
>>   	struct iphdr *ip;
>>
>> +	/* Don't allow non-TSO packets longer than MTU */
>> +	eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ?
>> +			VLAN_ETH_HLEN : ETH_HLEN;
>> +	if (!skb_is_gso(skb) &&
>> +	    (skb->len - eth_hdr_len) > adapter->netdev->mtu)
>> +			goto tx_drop;
>> +
>
> When you say 'cannot handle them', is it some kind of nasty thing like
> hang / crash ?
AFAIK, the firmware in the card becomes unresponsive and reboot is 
needed to make the NIC working.

Ivan
--
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
Sathya Perla - Oct. 16, 2013, 12:08 p.m.
> -----Original Message-----

> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]

> >

> > Pktgen can generate non-TSO frames of arbitrary length disregarding

> > the MTU value of the physical interface. Drop such frames in the driver

> > instead of sending them to HW as it cannot handle such frames.

> >

> > Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>

> > Signed-off-by: Sathya Perla <sathya.perla@emulex.com>

> > ---

> >  drivers/net/ethernet/emulex/benet/be_main.c |    9 +++++++--

> >  1 files changed, 7 insertions(+), 2 deletions(-)

> >

> > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c

> b/drivers/net/ethernet/emulex/benet/be_main.c

> > index 2c38cc4..76057b8 100644

> > --- a/drivers/net/ethernet/emulex/benet/be_main.c

> > +++ b/drivers/net/ethernet/emulex/benet/be_main.c

> > @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter

> *adapter,

> >  	unsigned int eth_hdr_len;

> >  	struct iphdr *ip;

> >

> > +	/* Don't allow non-TSO packets longer than MTU */

> > +	eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ?

> > +			VLAN_ETH_HLEN : ETH_HLEN;

> > +	if (!skb_is_gso(skb) &&

> > +	    (skb->len - eth_hdr_len) > adapter->netdev->mtu)

> > +			goto tx_drop;

> > +

> 

> When you say 'cannot handle them', is it some kind of nasty thing like

> hang / crash ?

This chip goes into an unrecoverable error state (needing a server reboot to rectify.)

> 

> One could imagine gso_size + sizeof(headers) > mtu, and give the same

> problem ?

Won't the mss (gso_size) value for TCP pkts be derived from the mtu of the out-going interface?
In that case when will gso_size + headers > mtu?
In any case, for TSO pkts, the HW just drops pkts when frames after segmentation
are longer than 9018bytes (or in recent FW versions 9200b)

> 

> 

> AFAIK, we have no check in net/core/dev.c.

> 

> Maybe we should instead add them there (and in pktgen)

I agree; doing this will solve issues with any other drivers/devices if they have a similar problem.

thanks,
-Sathya
Eric Dumazet - Oct. 16, 2013, 1:54 p.m.
On Wed, 2013-10-16 at 12:08 +0000, Sathya Perla wrote:

> Won't the mss (gso_size) value for TCP pkts be derived from the mtu of the out-going interface?
> In that case when will gso_size + headers > mtu?


Not on forwarding :(

Let see we have two ethernet devices, with different mtu : 1500 and 1400

GRO could aggregate GRO packets with MSS=1460 (gso_size=1460), then
we hapilly send this GSO packet to the other device, and apparently we
have no check against the lower mtu.

In the best case GRO packet is lost, but in some other cases, the device
hangs...

> In any case, for TSO pkts, the HW just drops pkts when frames after segmentation
> are longer than 9018bytes (or in recent FW versions 9200b)


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

Patch

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 2c38cc4..76057b8 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -855,6 +855,13 @@  static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter,
 	unsigned int eth_hdr_len;
 	struct iphdr *ip;
 
+	/* Don't allow non-TSO packets longer than MTU */
+	eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ?
+			VLAN_ETH_HLEN : ETH_HLEN;
+	if (!skb_is_gso(skb) &&
+	    (skb->len - eth_hdr_len) > adapter->netdev->mtu)
+			goto tx_drop;
+
 	/* Lancer, SH-R ASICs have a bug wherein Packets that are 32 bytes or less
 	 * may cause a transmit stall on that port. So the work-around is to
 	 * pad short packets (<= 32 bytes) to a 36-byte length.
@@ -869,8 +876,6 @@  static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter,
 	 * incorrecly when VLAN tag is inserted by HW.
 	 * For padded packets, Lancer computes incorrect checksum.
 	 */
-	eth_hdr_len = ntohs(skb->protocol) == ETH_P_8021Q ?
-						VLAN_ETH_HLEN : ETH_HLEN;
 	if (skb->len <= 60 &&
 	    (lancer_chip(adapter) || vlan_tx_tag_present(skb)) &&
 	    is_ipv4_pkt(skb)) {