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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 4 Mar 2012 21:25:07 -0800
From:	Stephen Hemminger <shemminger@...tta.com>
To:	Bart Van Assche <bvanassche@....org>
Cc:	Francois Romieu <romieu@...zoreil.com>, netdev@...r.kernel.org,
	Stephen Hemminger <shemminger@...l.org>,
	Jeff Garzik <jeff@...zik.org>
Subject: Re: [PATCH, RFC] skge: Fix race in tx path

On Mon, 27 Feb 2012 19:18:58 +0000
Bart Van Assche <bvanassche@....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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html