[PATCH, RFC] skge: Fix race in tx path

[PATCH, RFC] skge: Fix race in tx path

[Bart Van Assche]

Make sure that tx_ring.to_use is updated before skge_tx_done() reads it.

Fixes the following BUG on my setup:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000525
IP: [<ffffffff8130c95e>] net_rx_action+0x9e/0x290
Call Trace:
 [<ffffffff8104cfb1>] ? __do_softirq+0x81/0x250
 [<ffffffff8104cffd>] __do_softirq+0xcd/0x250
 [<ffffffff8104d297>] run_ksoftirqd+0x117/0x1e0
 [<ffffffff8104d180>] ? __do_softirq+0x250/0x250
 [<ffffffff81069786>] kthread+0x96/0xa0
 [<ffffffff813e8534>] kernel_thread_helper+0x4/0x10
 [<ffffffff813de75d>] ? retint_restore_args+0xe/0xe
 [<ffffffff810696f0>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff813e8530>] ? gs_change+0xb/0xb

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Stephen Hemminger <shemminger@osdl.org>
Cc: Jeff Garzik <jeff@garzik.org>
---
Note: I'm posting this patch as an RFC since I'm not a network driver expert.

 drivers/net/ethernet/marvell/skge.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c
index dea0cb4..2d805b9 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -2793,7 +2793,8 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 		tf->control |= BMU_EOF | BMU_IRQ_EOF;
 	}
 	/* Make sure all the descriptors written */
-	wmb();
+	skge->tx_ring.to_use = e->next;
+	smp_wmb();
 	td->control = BMU_OWN | BMU_SW | BMU_STF | control | len;
 	wmb();
 
@@ -2803,9 +2804,6 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 		     "tx queued, slot %td, len %d\n",
 		     e - skge->tx_ring.start, skb->len);
 
-	skge->tx_ring.to_use = e->next;
-	smp_wmb();
-
 	if (skge_avail(&skge->tx_ring) <= TX_LOW_WATER) {
 		netdev_dbg(dev, "transmit queue full\n");
 		netif_stop_queue(dev);
-- 
1.7.3.4

Re: [PATCH, RFC] skge: Fix race in tx path

[Francois Romieu]

Bart Van Assche <bvanassche@acm.org> :
[...]
> diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c
> index dea0cb4..2d805b9 100644
> --- a/drivers/net/ethernet/marvell/skge.c
> +++ b/drivers/net/ethernet/marvell/skge.c
> @@ -2793,7 +2793,8 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
>  		tf->control |= BMU_EOF | BMU_IRQ_EOF;
>  	}
>  	/* Make sure all the descriptors written */
> -	wmb();
> +	skge->tx_ring.to_use = e->next;
> +	smp_wmb();
>  	td->control = BMU_OWN | BMU_SW | BMU_STF | control | len;
>  	wmb();

With this change it seems possible for skge_tx_done to unmap a buffer before
it is sent when the requests for skge_xmit_frame are close enough.

-- 
Ueimor

Re: Re: [PATCH, RFC] skge: Fix race in tx path

[Bart Van Assche]

> With this change it seems possible for skge_tx_done to unmap a buffer
> before it is sent when the requests for skge_xmit_frame are close enough.

Thanks for the feedback. Does the patch below look better ? 

---
 drivers/net/ethernet/marvell/skge.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c
index 33947ac..55723b5 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -2800,15 +2800,15 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 
 	netdev_sent_queue(dev, skb->len);
 
+	skge->tx_ring.to_use = e->next;
+	smp_wmb();
+
 	skge_write8(hw, Q_ADDR(txqaddr[skge->port], Q_CSR), CSR_START);
 
 	netif_printk(skge, tx_queued, KERN_DEBUG, skge->netdev,
 		     "tx queued, slot %td, len %d\n",
 		     e - skge->tx_ring.start, skb->len);
 
-	skge->tx_ring.to_use = e->next;
-	smp_wmb();
-
 	if (skge_avail(&skge->tx_ring) <= TX_LOW_WATER) {
 		netdev_dbg(dev, "transmit queue full\n");
 		netif_stop_queue(dev);
-- 
1.7.3.4

Re: Re: [PATCH, RFC] skge: Fix race in tx path

[Francois Romieu]

Bart Van Assche <bvanassche@acm.org> :
> > With this change it seems possible for skge_tx_done to unmap a buffer
> > before it is sent when the requests for skge_xmit_frame are close enough.
> 
> Thanks for the feedback. Does the patch below look better ? 

Yes.

-- 
Ueimor

Re: [PATCH, RFC] skge: Fix race in tx path

[Stephen Hemminger]

On Mon, 27 Feb 2012 19:18:58 +0000
Bart Van Assche <bvanassche@acm.org> wrote:

> > With this change it seems possible for skge_tx_done to unmap a buffer
> > before it is sent when the requests for skge_xmit_frame are close enough.
> 
> Thanks for the feedback. Does the patch below look better ? 
> 
> ---
>  drivers/net/ethernet/marvell/skge.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c
> index 33947ac..55723b5 100644
> --- a/drivers/net/ethernet/marvell/skge.c
> +++ b/drivers/net/ethernet/marvell/skge.c
> @@ -2800,15 +2800,15 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
>  
>  	netdev_sent_queue(dev, skb->len);
>  
> +	skge->tx_ring.to_use = e->next;
> +	smp_wmb();
> +
>  	skge_write8(hw, Q_ADDR(txqaddr[skge->port], Q_CSR), CSR_START);
>  
>  	netif_printk(skge, tx_queued, KERN_DEBUG, skge->netdev,
>  		     "tx queued, slot %td, len %d\n",
>  		     e - skge->tx_ring.start, skb->len);
>  
> -	skge->tx_ring.to_use = e->next;
> -	smp_wmb();
> -
>  	if (skge_avail(&skge->tx_ring) <= TX_LOW_WATER) {
>  		netdev_dbg(dev, "transmit queue full\n");
>  		netif_stop_queue(dev);

I don't see how the problem you are seeing.
If the race happens with old code:
  
     ring element is setup and started.
        IRQ happens
                     ........ skge_tx_done()
                                starts cleaning
                                stops prematurely 
     ring_to_use is updated
     one skb stays stuck in ring until next TX clean

So the race would just leave one skb behind (until next transmit).
Looking at tg3, you will see similar code.
More likely mmiowb() is needd at end of tx.