netdev - Re: [PATCH] ethernet:arc: Fix racing of TX ring buffer

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20160515123829.GA2831@debian-dorm>
Date:	Sun, 15 May 2016 20:38:29 +0800
From:	Shuyu Wei <wsy2220@...il.com>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	wxt@...k-chips.com, davem@...emloft.net, heiko@...ech.de,
	linux-rockchip@...ts.infradead.org, netdev@...r.kernel.org
Subject: Re: [PATCH] ethernet:arc: Fix racing of TX ring buffer

On Sun, May 15, 2016 at 11:19:53AM +0200, Francois Romieu wrote:
> 
> static void arc_emac_tx_clean(struct net_device *ndev)
> {
> [...]
>         for (i = 0; i < TX_BD_NUM; i++) {
>                 unsigned int *txbd_dirty = &priv->txbd_dirty;
>                 struct arc_emac_bd *txbd = &priv->txbd[*txbd_dirty];
>                 struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
>                 struct sk_buff *skb = tx_buff->skb;
>                 unsigned int info = le32_to_cpu(txbd->info);
> 
>                 if ((info & FOR_EMAC) || !txbd->data || !skb)
>                         break;
>                         ^^^^^
> 
> -> the "break" statement prevents reading all txbds. At most one extra
>    descriptor is read and this driver isn't in the Mpps business.
> 

You are right, I forgot the break statement.

> > I tried your advice, Tx throughput can only reach 5.52MB/s.
> 
> Even with the original code above ?

Yes, I left tx_clean unmodified, and took your advice below.
I tested it again just now, this time throughput do reach 9.8MB/s,
Maybe last time cpu is not idle.

I still have a question, is it possible that tx_clean() run
between   priv->tx_buff[*txbd_curr].skb = skb   and   dma_wmb()?

--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -685,13 +685,15 @@ static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
        wmb();
 
        skb_tx_timestamp(skb);
+       priv->tx_buff[*txbd_curr].skb = skb;
+
+       dma_wmb();
 
        *info = cpu_to_le32(FOR_EMAC | FIRST_OR_LAST_MASK | len);
 
        /* Make sure info word is set */
        wmb();
 
-       priv->tx_buff[*txbd_curr].skb = skb;
 
        /* Increment index to point to the next BD */
        *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;