* Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.
2011-06-20 7:48 ` [PATCH 2/2] mv643xx_eth: fix race in trasmit path Richard Cochran
@ 2011-06-20 14:07 ` Maxime Bizon
2011-06-20 16:31 ` Eric Dumazet
2011-06-20 15:49 ` Eric Dumazet
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Maxime Bizon @ 2011-06-20 14:07 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, David Miller, Eric Dumazet, stable, Lennert Buytenhek
On Mon, 2011-06-20 at 09:48 +0200, Richard Cochran wrote:
Hi Richard,
> Because the socket buffer is freed in the completion interrupt, it is not
> safe to access it after submitting it to the hardware.
I don't see why.
skb is freed from txq_reclaim() which grabs the tx queue lock before,
(hence the lockless __skb_queue_xxx() in both functions)
What am I missing ?
> Cc: stable@kernel.org
> Cc: Lennert Buytenhek <buytenh@wantstofly.org>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
> drivers/net/mv643xx_eth.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
> index a5d9b1c..1b7d2c1 100644
> --- a/drivers/net/mv643xx_eth.c
> +++ b/drivers/net/mv643xx_eth.c
> @@ -859,7 +859,7 @@ no_csum:
> static netdev_tx_t mv643xx_eth_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> struct mv643xx_eth_private *mp = netdev_priv(dev);
> - int queue;
> + int length, queue;
> struct tx_queue *txq;
> struct netdev_queue *nq;
>
> @@ -881,10 +881,12 @@ static netdev_tx_t mv643xx_eth_xmit(struct sk_buff *skb, struct net_device *dev)
> return NETDEV_TX_OK;
> }
>
> + length = skb->len;
> +
> if (!txq_submit_skb(txq, skb)) {
> int entries_left;
>
> - txq->tx_bytes += skb->len;
> + txq->tx_bytes += length;
> txq->tx_packets++;
>
> entries_left = txq->tx_ring_size - txq->tx_desc_count;
--
Maxime
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.
2011-06-20 14:07 ` Maxime Bizon
@ 2011-06-20 16:31 ` Eric Dumazet
0 siblings, 0 replies; 12+ messages in thread
From: Eric Dumazet @ 2011-06-20 16:31 UTC (permalink / raw)
To: mbizon; +Cc: Richard Cochran, netdev, David Miller, stable, Lennert Buytenhek
Le lundi 20 juin 2011 à 16:07 +0200, Maxime Bizon a écrit :
> On Mon, 2011-06-20 at 09:48 +0200, Richard Cochran wrote:
>
> Hi Richard,
>
> > Because the socket buffer is freed in the completion interrupt, it is not
> > safe to access it after submitting it to the hardware.
>
> I don't see why.
>
> skb is freed from txq_reclaim() which grabs the tx queue lock before,
> (hence the lockless __skb_queue_xxx() in both functions)
>
> What am I missing ?
You are right, this driver still takes tx queue lock in its TX
completion path.
txq_reclaim() can currently run for a very long time, blocking other
cpus to make progress if blocked on tx queue lock.
So consider this patch as a cleanup :
Its better to make all drivers behave the same way
1) Either increment tx stats in their start_xmit(), and making sure they
dont access skb->len too late.
2) increment tx stats in their TX completion path.
Then we can work on making TX completion path not taking tx queue lock
in its fast path.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.
2011-06-20 7:48 ` [PATCH 2/2] mv643xx_eth: fix race in trasmit path Richard Cochran
2011-06-20 14:07 ` Maxime Bizon
@ 2011-06-20 15:49 ` Eric Dumazet
2011-06-20 16:33 ` Lennert Buytenhek
2011-06-21 23:00 ` David Miller
3 siblings, 0 replies; 12+ messages in thread
From: Eric Dumazet @ 2011-06-20 15:49 UTC (permalink / raw)
To: Richard Cochran; +Cc: netdev, David Miller, stable, Lennert Buytenhek
Le lundi 20 juin 2011 à 09:48 +0200, Richard Cochran a écrit :
> Because the socket buffer is freed in the completion interrupt, it is not
> safe to access it after submitting it to the hardware.
>
> Cc: stable@kernel.org
> Cc: Lennert Buytenhek <buytenh@wantstofly.org>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
> drivers/net/mv643xx_eth.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.
2011-06-20 7:48 ` [PATCH 2/2] mv643xx_eth: fix race in trasmit path Richard Cochran
2011-06-20 14:07 ` Maxime Bizon
2011-06-20 15:49 ` Eric Dumazet
@ 2011-06-20 16:33 ` Lennert Buytenhek
2011-06-20 17:19 ` Eric Dumazet
2011-06-21 23:00 ` David Miller
3 siblings, 1 reply; 12+ messages in thread
From: Lennert Buytenhek @ 2011-06-20 16:33 UTC (permalink / raw)
To: Richard Cochran; +Cc: netdev, David Miller, Eric Dumazet, stable
On Mon, Jun 20, 2011 at 09:48:07AM +0200, Richard Cochran wrote:
> Because the socket buffer is freed in the completion interrupt, it
> is not safe to access it after submitting it to the hardware.
Maybe I'm missing something here, but mv643xx_eth TX reclaim is done
from NAPI poll, under __netif_tx_lock(), while mv643xx_eth_xmit() also
runs under __netif_tx_lock().
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.
2011-06-20 16:33 ` Lennert Buytenhek
@ 2011-06-20 17:19 ` Eric Dumazet
2011-06-20 17:21 ` Eric Dumazet
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2011-06-20 17:19 UTC (permalink / raw)
To: Lennert Buytenhek; +Cc: Richard Cochran, netdev, David Miller, stable
Le lundi 20 juin 2011 à 18:33 +0200, Lennert Buytenhek a écrit :
> On Mon, Jun 20, 2011 at 09:48:07AM +0200, Richard Cochran wrote:
>
> > Because the socket buffer is freed in the completion interrupt, it
> > is not safe to access it after submitting it to the hardware.
>
> Maybe I'm missing something here, but mv643xx_eth TX reclaim is done
> from NAPI poll, under __netif_tx_lock(), while mv643xx_eth_xmit() also
> runs under __netif_tx_lock().
See my previous answer. Its true this driver _currently_ holds tx queue
lock in its TX completion. But that might/should change.
Goal is to make tx completion not use tx queue lock in fast path, like
its done in tg3, bnx2, bnx2x ... and other recent drivers.
Its obviously correct to move skb->len access in start_xmit() before
starting the IO, even if not a bug fix, it makes all drivers behave the
same : When reviewing them, its easier not to worry about these possible
use after free.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.
2011-06-20 17:19 ` Eric Dumazet
@ 2011-06-20 17:21 ` Eric Dumazet
0 siblings, 0 replies; 12+ messages in thread
From: Eric Dumazet @ 2011-06-20 17:21 UTC (permalink / raw)
To: Lennert Buytenhek; +Cc: Richard Cochran, netdev, David Miller, stable
Le lundi 20 juin 2011 à 19:19 +0200, Eric Dumazet a écrit :
> Le lundi 20 juin 2011 à 18:33 +0200, Lennert Buytenhek a écrit :
> > On Mon, Jun 20, 2011 at 09:48:07AM +0200, Richard Cochran wrote:
> >
> > > Because the socket buffer is freed in the completion interrupt, it
> > > is not safe to access it after submitting it to the hardware.
> >
> > Maybe I'm missing something here, but mv643xx_eth TX reclaim is done
> > from NAPI poll, under __netif_tx_lock(), while mv643xx_eth_xmit() also
> > runs under __netif_tx_lock().
>
> See my previous answer. Its true this driver _currently_ holds tx queue
> lock in its TX completion. But that might/should change.
>
> Goal is to make tx completion not use tx queue lock in fast path, like
> its done in tg3, bnx2, bnx2x ... and other recent drivers.
>
> Its obviously correct to move skb->len access in start_xmit() before
> starting the IO, even if not a bug fix, it makes all drivers behave the
> same : When reviewing them, its easier not to worry about these possible
> use after free.
>
>
One random example found in drivers/staging/hv/netvsc_drv.c
Is the "net->stats.tx_bytes += skb->len;" found in line 188 is correct ?
Who knows ?
I believe its not correct ;)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.
2011-06-20 7:48 ` [PATCH 2/2] mv643xx_eth: fix race in trasmit path Richard Cochran
` (2 preceding siblings ...)
2011-06-20 16:33 ` Lennert Buytenhek
@ 2011-06-21 23:00 ` David Miller
3 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2011-06-21 23:00 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, eric.dumazet, stable, buytenh
From: Richard Cochran <richardcochran@gmail.com>
Date: Mon, 20 Jun 2011 09:48:07 +0200
> Because the socket buffer is freed in the completion interrupt, it is not
> safe to access it after submitting it to the hardware.
>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
Since this isn't actually a bonafide bug fix I've applied this to net-next-2.6
^ permalink raw reply [flat|nested] 12+ messages in thread