[PATCH] gianfar: fix potential sk_wmem_alloc imbalance

[PATCH] gianfar: fix potential sk_wmem_alloc imbalance

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

commit db83d136d7f753 (gianfar: Fix missing sock reference when
processing TX time stamps) added a potential sk_wmem_alloc imbalance

If the new skb has a different truesize than old one, we can get a
negative sk_wmem_alloc once new skb is orphaned at TX completion.

Now we no longer early orphan skbs in dev_hard_start_xmit(), this
probably can lead to fatal bugs.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Manfred Rudigier <manfred.rudigier@omicron.at>
Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
Cc: Jiajun Wu <b06378@freescale.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Andy Fleming <afleming@freescale.com>
---

Note : I don't have the hardware and discovered this problem by code
analysis. So please compile and run this patch before Acking it,
thanks !

BTW, dev->needed_headroom should be set to GMAC_FCB_LEN + GMAC_TXPAL_LEN
to avoid reallocations...

 drivers/net/ethernet/freescale/gianfar.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index f2db8fc..ab1d80f 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2063,10 +2063,9 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			return NETDEV_TX_OK;
 		}
 
-		/* Steal sock reference for processing TX time stamps */
-		swap(skb_new->sk, skb->sk);
-		swap(skb_new->destructor, skb->destructor);
-		kfree_skb(skb);
+		if (skb->sk)
+			skb_set_owner_w(skb_new, skb->sk);
+		consume_skb(skb);
 		skb = skb_new;
 	}
 

Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance

[Paul Gortmaker]

[[PATCH] gianfar: fix potential sk_wmem_alloc imbalance] On 05/07/2012 (Thu 23:45) Eric Dumazet wrote:

> From: Eric Dumazet <edumazet@google.com>
> 
> commit db83d136d7f753 (gianfar: Fix missing sock reference when
> processing TX time stamps) added a potential sk_wmem_alloc imbalance
> 
> If the new skb has a different truesize than old one, we can get a
> negative sk_wmem_alloc once new skb is orphaned at TX completion.
> 
> Now we no longer early orphan skbs in dev_hard_start_xmit(), this
> probably can lead to fatal bugs.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Manfred Rudigier <manfred.rudigier@omicron.at>
> Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
> Cc: Jiajun Wu <b06378@freescale.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Andy Fleming <afleming@freescale.com>
> ---
> 
> Note : I don't have the hardware and discovered this problem by code
> analysis. So please compile and run this patch before Acking it,
> thanks !

I can do that on Monday when I'm back in the office if nobody else has
already done it by then.

> 
> BTW, dev->needed_headroom should be set to GMAC_FCB_LEN + GMAC_TXPAL_LEN
> to avoid reallocations...

Aside from the one line change at driver init, is there more to it than
that?  More specifically, it currently does:

fcb_length = GMAC_FCB_LEN;

if (...timestamps...)
	fcb_length = GMAC_FCB_LEN + GMAC_TXPAL_LEN;

if (... && (skb_headroom(skb) < fcb_length))
	...
	skb_new = skb_realloc_headroom(skb, fcb_length);

and I don't know the code well enough to know if setting the
needed_headroom value _guarantees_ the above fcb_length comparison
will always be false, and hence can be deleted.  It kind of looks
like it via LL_RESERVED_SPACE, but I'm not 100% sure...

Thanks,
Paul.
--

> 
>  drivers/net/ethernet/freescale/gianfar.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index f2db8fc..ab1d80f 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2063,10 +2063,9 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  			return NETDEV_TX_OK;
>  		}
>  
> -		/* Steal sock reference for processing TX time stamps */
> -		swap(skb_new->sk, skb->sk);
> -		swap(skb_new->destructor, skb->destructor);
> -		kfree_skb(skb);
> +		if (skb->sk)
> +			skb_set_owner_w(skb_new, skb->sk);
> +		consume_skb(skb);
>  		skb = skb_new;
>  	}
>  
> 
> 

Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance

[Eric Dumazet]

On Fri, 2012-07-06 at 14:09 -0400, Paul Gortmaker wrote:

> Aside from the one line change at driver init, is there more to it than
> that?  More specifically, it currently does:
> 
> fcb_length = GMAC_FCB_LEN;
> 
> if (...timestamps...)
> 	fcb_length = GMAC_FCB_LEN + GMAC_TXPAL_LEN;
> 
> if (... && (skb_headroom(skb) < fcb_length))
> 	...
> 	skb_new = skb_realloc_headroom(skb, fcb_length);
> 
> and I don't know the code well enough to know if setting the
> needed_headroom value _guarantees_ the above fcb_length comparison
> will always be false, and hence can be deleted.  It kind of looks
> like it via LL_RESERVED_SPACE, but I'm not 100% sure...

This is not a guarantee but should take care of most cases.

So we should keep the test anyway.

And init needed_headroom to the largest room. Existing tests seems to
ignore vlan case and timestamping.

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index af16f9f..b4517b7 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1084,9 +1084,7 @@ static int gfar_probe(struct platform_device *ofdev)
 	else
 		priv->padding = 0;
 
-	if (dev->features & NETIF_F_IP_CSUM ||
-	    priv->device_flags & FSL_GIANFAR_DEV_HAS_TIMER)
-		dev->needed_headroom = GMAC_FCB_LEN;
+	dev->needed_headroom = GMAC_FCB_LEN + GMAC_TXPAL_LEN;
 
 	/* Program the isrg regs only if number of grps > 1 */
 	if (priv->num_grps > 1) {

Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance

[Paul Gortmaker]

On 12-07-05 05:45 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> commit db83d136d7f753 (gianfar: Fix missing sock reference when
> processing TX time stamps) added a potential sk_wmem_alloc imbalance
> 
> If the new skb has a different truesize than old one, we can get a
> negative sk_wmem_alloc once new skb is orphaned at TX completion.
> 
> Now we no longer early orphan skbs in dev_hard_start_xmit(), this
> probably can lead to fatal bugs.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Manfred Rudigier <manfred.rudigier@omicron.at>
> Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
> Cc: Jiajun Wu <b06378@freescale.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Andy Fleming <afleming@freescale.com>
> ---
> 
> Note : I don't have the hardware and discovered this problem by code
> analysis. So please compile and run this patch before Acking it,
> thanks !

Compiles clean and boot tested with NFS rootfs on mpc8315erdb board
defconfig on net-next [5c9df5fed19 "small cleanup in ax25_addr_parse"]

> 
> BTW, dev->needed_headroom should be set to GMAC_FCB_LEN + GMAC_TXPAL_LEN
> to avoid reallocations...

I also layered on the patch you sent for this and rebuilt, and it
too passes the same basic sanity test, so feel free to add a
Tested-by or similar to your headroom fix as well.

Paul.

> 
>  drivers/net/ethernet/freescale/gianfar.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index f2db8fc..ab1d80f 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2063,10 +2063,9 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  			return NETDEV_TX_OK;
>  		}
>  
> -		/* Steal sock reference for processing TX time stamps */
> -		swap(skb_new->sk, skb->sk);
> -		swap(skb_new->destructor, skb->destructor);
> -		kfree_skb(skb);
> +		if (skb->sk)
> +			skb_set_owner_w(skb_new, skb->sk);
> +		consume_skb(skb);
>  		skb = skb_new;
>  	}
>  
> 
> 

Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 05 Jul 2012 23:45:13 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> commit db83d136d7f753 (gianfar: Fix missing sock reference when
> processing TX time stamps) added a potential sk_wmem_alloc imbalance
> 
> If the new skb has a different truesize than old one, we can get a
> negative sk_wmem_alloc once new skb is orphaned at TX completion.
> 
> Now we no longer early orphan skbs in dev_hard_start_xmit(), this
> probably can lead to fatal bugs.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Manfred Rudigier <manfred.rudigier@omicron.at>
> Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
> Cc: Jiajun Wu <b06378@freescale.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Andy Fleming <afleming@freescale.com>

Now that this has been tested by Paul, I've applied it.

Thanks.

Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance

[Claudiu Manoil]

Hi,

On 7/8/2012 12:09 PM, Eric Dumazet wrote:
> On Fri, 2012-07-06 at 14:09 -0400, Paul Gortmaker wrote:
>
>> Aside from the one line change at driver init, is there more to it than
>> that?  More specifically, it currently does:
>>
>> fcb_length = GMAC_FCB_LEN;
>>
>> if (...timestamps...)
>> 	fcb_length = GMAC_FCB_LEN + GMAC_TXPAL_LEN;
>>
>> if (... && (skb_headroom(skb) < fcb_length))
>> 	...
>> 	skb_new = skb_realloc_headroom(skb, fcb_length);
>>
>> and I don't know the code well enough to know if setting the
>> needed_headroom value _guarantees_ the above fcb_length comparison
>> will always be false, and hence can be deleted.  It kind of looks
>> like it via LL_RESERVED_SPACE, but I'm not 100% sure...
>
> This is not a guarantee but should take care of most cases.
>
> So we should keep the test anyway.
>
> And init needed_headroom to the largest room. Existing tests seems to
> ignore vlan case and timestamping.

VLAN tagging (offloaded by eTSEC) is accommodated by the FCB block (8 
bytes), which seemingly has already been reserved in the headroom 
(GMAC_FCB_LEN). So it shouldn't require extra headroom.

And, according to the hw manual, time stamp insertion for transmit 
packets requires an additional 16 bytes, next to the FCB, as the 
timestamp (8 bytes) is written at an offset of 16. And looks like this 
is taken care of by GMAC_TXPAL_LEN.

However I don't fully understand the "needed_headroom" setting and its 
implications, maybe you could help with the following questions.
Is "needed_headroom" setting relevant for IP forwarding scenarios too?
Or is it used only in the case when the networking stack generates 
packets for Tx?

Thanks!

Claudiu

>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index af16f9f..b4517b7 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -1084,9 +1084,7 @@ static int gfar_probe(struct platform_device *ofdev)
>   	else
>   		priv->padding = 0;
>
> -	if (dev->features & NETIF_F_IP_CSUM ||
> -	    priv->device_flags & FSL_GIANFAR_DEV_HAS_TIMER)
> -		dev->needed_headroom = GMAC_FCB_LEN;
> +	dev->needed_headroom = GMAC_FCB_LEN + GMAC_TXPAL_LEN;
>
>   	/* Program the isrg regs only if number of grps > 1 */
>   	if (priv->num_grps > 1) {
>
>
>
>
> .
>