* [PATCH] gianfar: fix potential sk_wmem_alloc imbalance
@ 2012-07-05 21:45 Eric Dumazet
2012-07-06 18:09 ` Paul Gortmaker
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eric Dumazet @ 2012-07-05 21:45 UTC (permalink / raw)
To: David Miller
Cc: netdev, Manfred Rudigier, Claudiu Manoil, Jiajun Wu,
Paul Gortmaker, Andy Fleming
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance
2012-07-05 21:45 [PATCH] gianfar: fix potential sk_wmem_alloc imbalance Eric Dumazet
@ 2012-07-06 18:09 ` Paul Gortmaker
2012-07-08 9:09 ` Eric Dumazet
2012-07-09 21:38 ` Paul Gortmaker
2012-07-09 22:28 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Paul Gortmaker @ 2012-07-06 18:09 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Manfred Rudigier, Claudiu Manoil, Jiajun Wu,
Andy Fleming
[[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;
> }
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance
2012-07-06 18:09 ` Paul Gortmaker
@ 2012-07-08 9:09 ` Eric Dumazet
2012-07-11 8:06 ` Claudiu Manoil
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2012-07-08 9:09 UTC (permalink / raw)
To: Paul Gortmaker
Cc: David Miller, netdev, Manfred Rudigier, Claudiu Manoil, Jiajun Wu,
Andy Fleming
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) {
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance
2012-07-08 9:09 ` Eric Dumazet
@ 2012-07-11 8:06 ` Claudiu Manoil
0 siblings, 0 replies; 6+ messages in thread
From: Claudiu Manoil @ 2012-07-11 8:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: Paul Gortmaker, David Miller, netdev, Manfred Rudigier, Jiajun Wu,
Andy Fleming
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) {
>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance
2012-07-05 21:45 [PATCH] gianfar: fix potential sk_wmem_alloc imbalance Eric Dumazet
2012-07-06 18:09 ` Paul Gortmaker
@ 2012-07-09 21:38 ` Paul Gortmaker
2012-07-09 22:28 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Paul Gortmaker @ 2012-07-09 21:38 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Manfred Rudigier, Claudiu Manoil, Jiajun Wu,
Andy Fleming
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;
> }
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gianfar: fix potential sk_wmem_alloc imbalance
2012-07-05 21:45 [PATCH] gianfar: fix potential sk_wmem_alloc imbalance Eric Dumazet
2012-07-06 18:09 ` Paul Gortmaker
2012-07-09 21:38 ` Paul Gortmaker
@ 2012-07-09 22:28 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2012-07-09 22:28 UTC (permalink / raw)
To: eric.dumazet
Cc: netdev, manfred.rudigier, claudiu.manoil, b06378, paul.gortmaker,
afleming
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-11 8:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-05 21:45 [PATCH] gianfar: fix potential sk_wmem_alloc imbalance Eric Dumazet
2012-07-06 18:09 ` Paul Gortmaker
2012-07-08 9:09 ` Eric Dumazet
2012-07-11 8:06 ` Claudiu Manoil
2012-07-09 21:38 ` Paul Gortmaker
2012-07-09 22:28 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).