| 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
| ||
|
Message-ID: <571A4D36.2000506@boundarydevices.com>
Date: Fri, 22 Apr 2016 09:11:34 -0700
From: Troy Kisky <troy.kisky@...ndarydevices.com>
To: Fugang Duan <fugang.duan@....com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"lznuaa@...il.com" <lznuaa@...il.com>
Cc: Fabio Estevam <fabio.estevam@....com>,
"l.stach@...gutronix.de" <l.stach@...gutronix.de>,
"andrew@...n.ch" <andrew@...n.ch>,
"tremyfr@...il.com" <tremyfr@...il.com>,
"gerg@...inux.org" <gerg@...inux.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"johannes@...solutions.net" <johannes@...solutions.net>,
"stillcompiling@...il.com" <stillcompiling@...il.com>,
"sergei.shtylyov@...entembedded.com"
<sergei.shtylyov@...entembedded.com>,
"arnd@...db.de" <arnd@...db.de>,
"holgerschurig@...il.com" <holgerschurig@...il.com>
Subject: Re: [PATCH net 1/1] net: fec: update dirty_tx even if no skb
On 4/21/2016 10:59 PM, Fugang Duan wrote:
> From: Troy Kisky <troy.kisky@...ndarydevices.com> Sent: Friday, April 22, 2016 10:01 AM
>> To: netdev@...r.kernel.org; davem@...emloft.net; Fugang Duan
>> <fugang.duan@....com>; lznuaa@...il.com
>> Cc: Fabio Estevam <fabio.estevam@....com>; l.stach@...gutronix.de;
>> andrew@...n.ch; tremyfr@...il.com; gerg@...inux.org; linux-arm-
>> kernel@...ts.infradead.org; johannes@...solutions.net;
>> stillcompiling@...il.com; sergei.shtylyov@...entembedded.com;
>> arnd@...db.de; holgerschurig@...il.com; Troy Kisky
>> <troy.kisky@...ndarydevices.com>
>> Subject: [PATCH net 1/1] net: fec: update dirty_tx even if no skb
>>
>> If dirty_tx isn't updated, then dma_unmap_single will be called twice.
>>
>> This fixes a
>> [ 58.420980] ------------[ cut here ]------------
>> [ 58.425667] WARNING: CPU: 0 PID: 377 at /home/schurig/d/mkarm/linux-
>> 4.5/lib/dma-debug.c:1096 check_unmap+0x9d0/0xab8()
>> [ 58.436405] fec 2188000.ethernet: DMA-API: device driver tries to free DMA
>> memory it has not allocated [device address=0x0000000000000000] [size=66
>> bytes]
>>
>> encountered by Holger
>>
>> Signed-off-by: Troy Kisky <troy.kisky@...ndarydevices.com>
>> Tested-by: <holgerschurig@...il.com>
>> ---
>> drivers/net/ethernet/freescale/fec_main.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 08243c2..b71654c 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1197,10 +1197,8 @@ fec_enet_tx_queue(struct net_device *ndev, u16
>> queue_id)
>> fec16_to_cpu(bdp->cbd_datlen),
>> DMA_TO_DEVICE);
>> bdp->cbd_bufaddr = cpu_to_fec32(0);
>> - if (!skb) {
>> - bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
>> - continue;
>> - }
>> + if (!skb)
>> + goto skb_done;
>>
>> /* Check for errors. */
>> if (status & (BD_ENET_TX_HB | BD_ENET_TX_LC | @@ -1239,7
>> +1237,7 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>>
>> /* Free the sk buffer associated with this last transmit */
>> dev_kfree_skb_any(skb);
>> -
>> +skb_done:
>> /* Make sure the update to bdp and tx_skbuff are performed
>> * before dirty_tx
>> */
>> --
>> 2.5.0
>
> The patch is fine for me.
> Can you review below patch that also fix the issue. It can take much effort due to less rmb() and READ_ONCE() operation that is very sensitive for duplex Gbps test for i.MX6SX/i.MX7d SOC. (i.MX6SX can reach at 1.4Gbps, i.MX7D can reach at 1.8Gbps.)
If "READ_ONCE(bdp->cbd_sc)" is really that expensive, then you should skip the 1st read as well and
only do it after skb presence is verified. But some numbers would really help sell the patch.
Also, I comment as to why the code is reorganized would be warranted
>
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1160,12 +1160,13 @@ static void
> fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> {
> struct fec_enet_private *fep;
> - struct bufdesc *bdp;
> + struct bufdesc *bdp, *bdp_t;
> unsigned short status;
> struct sk_buff *skb;
> struct fec_enet_priv_tx_q *txq;
> struct netdev_queue *nq;
> int index = 0;
> + int i, bdnum;
> int entries_free;
>
> fep = netdev_priv(ndev);
> @@ -1187,20 +1188,28 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
> if (status & BD_ENET_TX_READY)
> break;
>
> - index = fec_enet_get_bd_index(bdp, &txq->bd);
> -
> + bdp_t = bdp;
> + bdnum = 1;
> + index = fec_enet_get_bd_index(txq->tx_bd_base, bdp_t, fep);
> skb = txq->tx_skbuff[index];
> - txq->tx_skbuff[index] = NULL;
> - if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr)))
> - dma_unmap_single(&fep->pdev->dev,
> - fec32_to_cpu(bdp->cbd_bufaddr),
> - fec16_to_cpu(bdp->cbd_datlen),
> - DMA_TO_DEVICE);
> - bdp->cbd_bufaddr = cpu_to_fec32(0);
> - if (!skb) {
> - bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
> - continue;
> + while (!skb) {
> + bdp_t = fec_enet_get_nextdesc(bdp_t, &txq->bd);
> + index = fec_enet_get_bd_index(txq->tx_bd_base, bdp_t, fep);
> + skb = txq->tx_skbuff[index];
> + bdnum++;
> + }
> + status = fec16_to_cpu(READ_ONCE(bdp->cbd_sc));
> + if (status & BD_ENET_TX_READY)
> + break;
> +
> + for (i = 0; i < bdnum; i++) {
> + if (!IS_TSO_HEADER(txq, bdp->cbd_bufaddr))
> + dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
> + bdp->cbd_datlen, DMA_TO_DEVICE);
> + bdp->cbd_bufaddr = 0;
> + if (i < bdnum - 1)
> + bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
> }
> + txq->tx_skbuff[index] = NULL;
>
>
> Regards,
> Andy
>
Powered by blists - more mailing lists