* [PATCH] net: fec: fix potential use after free
@ 2013-12-19 18:53 Eric Dumazet
2013-12-19 19:32 ` Richard Cochran
2013-12-19 23:31 ` David Miller
0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2013-12-19 18:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Frank Li, Richard Cochran
From: Eric Dumazet <edumazet@google.com>
skb_tx_timestamp(skb) should be called _before_ TX completion
has a chance to trigger, otherwise it is too late and we access
freed memory.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
Cc: Frank Li <Frank.Li@freescale.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/freescale/fec_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index e7c8b749c5a5..50bb71c663e2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -428,6 +428,8 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
/* If this was the last BD in the ring, start at the beginning again. */
bdp = fec_enet_get_nextdesc(bdp, fep);
+ skb_tx_timestamp(skb);
+
fep->cur_tx = bdp;
if (fep->cur_tx == fep->dirty_tx)
@@ -436,8 +438,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
/* Trigger transmission start */
writel(0, fep->hwp + FEC_X_DES_ACTIVE);
- skb_tx_timestamp(skb);
-
return NETDEV_TX_OK;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] net: fec: fix potential use after free
2013-12-19 18:53 [PATCH] net: fec: fix potential use after free Eric Dumazet
@ 2013-12-19 19:32 ` Richard Cochran
2013-12-19 19:55 ` Frank.Li
` (2 more replies)
2013-12-19 23:31 ` David Miller
1 sibling, 3 replies; 9+ messages in thread
From: Richard Cochran @ 2013-12-19 19:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Frank Li
On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> skb_tx_timestamp(skb) should be called _before_ TX completion
> has a chance to trigger, otherwise it is too late and we access
> freed memory.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
So the hw_lock spin lock was protecting against this issue?
Nice catch. How ever did you find this?
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] net: fec: fix potential use after free
2013-12-19 19:32 ` Richard Cochran
@ 2013-12-19 19:55 ` Frank.Li
2013-12-19 20:03 ` Eric Dumazet
2013-12-19 20:05 ` Frank Li
2 siblings, 0 replies; 9+ messages in thread
From: Frank.Li @ 2013-12-19 19:55 UTC (permalink / raw)
To: Richard Cochran, Eric Dumazet; +Cc: David Miller, netdev
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, December 19, 2013 1:33 PM
> To: Eric Dumazet
> Cc: David Miller; netdev; Li Frank-B20596
> Subject: Re: [PATCH] net: fec: fix potential use after free
>
> On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > skb_tx_timestamp(skb) should be called _before_ TX completion has a
> > chance to trigger, otherwise it is too late and we access freed
> > memory.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix
> > dead lock")
>
> So the hw_lock spin lock was protecting against this issue?
>
> Nice catch. How ever did you find this?
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
>
Acked-by: Frank Li <Frank.Li@freescale.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] net: fec: fix potential use after free
2013-12-19 19:32 ` Richard Cochran
2013-12-19 19:55 ` Frank.Li
@ 2013-12-19 20:03 ` Eric Dumazet
2013-12-19 20:05 ` Eric Dumazet
2013-12-19 20:05 ` Frank Li
2 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2013-12-19 20:03 UTC (permalink / raw)
To: Richard Cochran; +Cc: David Miller, netdev, Frank Li
On Thu, 2013-12-19 at 20:32 +0100, Richard Cochran wrote:
> On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > skb_tx_timestamp(skb) should be called _before_ TX completion
> > has a chance to trigger, otherwise it is too late and we access
> > freed memory.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
>
> So the hw_lock spin lock was protecting against this issue?
>
> Nice catch. How ever did you find this?
While looking at another driver, founding the same issue.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] net: fec: fix potential use after free
2013-12-19 19:32 ` Richard Cochran
2013-12-19 19:55 ` Frank.Li
2013-12-19 20:03 ` Eric Dumazet
@ 2013-12-19 20:05 ` Frank Li
2013-12-19 20:18 ` Eric Dumazet
2 siblings, 1 reply; 9+ messages in thread
From: Frank Li @ 2013-12-19 20:05 UTC (permalink / raw)
To: Richard Cochran; +Cc: Eric Dumazet, David Miller, netdev, Frank Li
On Thu, Dec 19, 2013 at 1:32 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
> On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> skb_tx_timestamp(skb) should be called _before_ TX completion
>> has a chance to trigger, otherwise it is too late and we access
>> freed memory.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
>
> So the hw_lock spin lock was protecting against this issue?
>
> Nice catch. How ever did you find this?
I think It is not related with hw_lock. After trigger transfer,
there are possibility that tx completed immediately and irq handle
will free skb.
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
> --
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] net: fec: fix potential use after free
2013-12-19 20:05 ` Frank Li
@ 2013-12-19 20:18 ` Eric Dumazet
2013-12-19 20:30 ` Frank.Li
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2013-12-19 20:18 UTC (permalink / raw)
To: Frank Li; +Cc: Richard Cochran, David Miller, netdev, Frank Li
On Thu, 2013-12-19 at 14:05 -0600, Frank Li wrote:
> I think It is not related with hw_lock. After trigger transfer,
> there are possibility that tx completed immediately and irq handle
> will free skb.
Well, before the de5fb0a05348 commit, tx completion and start_xmit()
could not run in //
So hw_lock was effectively preventing the issue.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] net: fec: fix potential use after free
2013-12-19 20:18 ` Eric Dumazet
@ 2013-12-19 20:30 ` Frank.Li
0 siblings, 0 replies; 9+ messages in thread
From: Frank.Li @ 2013-12-19 20:30 UTC (permalink / raw)
To: Eric Dumazet, Frank Li; +Cc: Richard Cochran, David Miller, netdev
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Thursday, December 19, 2013 2:19 PM
> To: Frank Li
> Cc: Richard Cochran; David Miller; netdev; Li Frank-B20596
> Subject: Re: [PATCH] net: fec: fix potential use after free
>
> On Thu, 2013-12-19 at 14:05 -0600, Frank Li wrote:
>
> > I think It is not related with hw_lock. After trigger transfer, there
> > are possibility that tx completed immediately and irq handle will free
> > skb.
>
> Well, before the de5fb0a05348 commit, tx completion and start_xmit() could not
> run in //
>
> So hw_lock was effectively preventing the issue.
>
Yes, IRQ handle will wait for spin_lock.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: fec: fix potential use after free
2013-12-19 18:53 [PATCH] net: fec: fix potential use after free Eric Dumazet
2013-12-19 19:32 ` Richard Cochran
@ 2013-12-19 23:31 ` David Miller
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2013-12-19 23:31 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, Frank.Li, richardcochran
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 Dec 2013 10:53:02 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> skb_tx_timestamp(skb) should be called _before_ TX completion
> has a chance to trigger, otherwise it is too late and we access
> freed memory.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-12-19 23:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-19 18:53 [PATCH] net: fec: fix potential use after free Eric Dumazet
2013-12-19 19:32 ` Richard Cochran
2013-12-19 19:55 ` Frank.Li
2013-12-19 20:03 ` Eric Dumazet
2013-12-19 20:05 ` Eric Dumazet
2013-12-19 20:05 ` Frank Li
2013-12-19 20:18 ` Eric Dumazet
2013-12-19 20:30 ` Frank.Li
2013-12-19 23:31 ` 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).