[PATCH 0/2] fix tiny races in MAC drivers

[PATCH 0/2] fix tiny races in MAC drivers

[Richard Cochran]

In course of adding transmit time stamping, some unrelated problems
were uncovered. Drivers which free transmit buffers in an ISR must not
access the buffers after giving them to the hardware.

Richard Cochran (2):
  pxa168_eth: fix race in transmit path.
  mv643xx_eth: fix race in trasmit path.

 drivers/net/mv643xx_eth.c |    6 ++++--
 drivers/net/pxa168_eth.c  |    2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

[PATCH 1/2] pxa168_eth: fix race in transmit path.

[Richard Cochran]

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: Sachin Sanap <ssanap@marvell.com>
Cc: Zhangfei Gao <zgao6@marvell.com>
Cc: Philip Rakity <prakity@marvell.com>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/pxa168_eth.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/pxa168_eth.c b/drivers/net/pxa168_eth.c
index 89f7540..5f597ca 100644
--- a/drivers/net/pxa168_eth.c
+++ b/drivers/net/pxa168_eth.c
@@ -1273,7 +1273,7 @@ static int pxa168_eth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	wmb();
 	wrl(pep, SDMA_CMD, SDMA_CMD_TXDH | SDMA_CMD_ERD);
 
-	stats->tx_bytes += skb->len;
+	stats->tx_bytes += length;
 	stats->tx_packets++;
 	dev->trans_start = jiffies;
 	if (pep->tx_ring_size - pep->tx_desc_count <= 1) {
-- 
1.7.0.4

[PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[Richard Cochran]

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

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

Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[Maxime Bizon]

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

Re: [PATCH 1/2] pxa168_eth: fix race in transmit path.

[Eric Dumazet]

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: Sachin Sanap <ssanap@marvell.com>
> Cc: Zhangfei Gao <zgao6@marvell.com>
> Cc: Philip Rakity <prakity@marvell.com>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[Eric Dumazet]

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>

Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[Eric Dumazet]

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.

Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[Lennert Buytenhek]

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().

Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[Eric Dumazet]

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.

Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[Eric Dumazet]

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

Re: [PATCH 1/2] pxa168_eth: fix race in transmit path.

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 20 Jun 2011 17:49:17 +0200

> 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: Sachin Sanap <ssanap@marvell.com>
>> Cc: Zhangfei Gao <zgao6@marvell.com>
>> Cc: Philip Rakity <prakity@marvell.com>
>> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

Re: [PATCH 2/2] mv643xx_eth: fix race in trasmit path.

[David Miller]

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