netdev - Re: [BUG] bcmgenet 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <56EAE2A8.5000206@gmail.com>
Date:	Thu, 17 Mar 2016 10:00:24 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, opendmb@...il.com
Subject: Re: [BUG] bcmgenet tx path

Hi Eric,

On 16/03/16 07:18, Eric Dumazet wrote:
> Hi Florian
> 
> I was looking at drivers/net/ethernet/broadcom/genet/bcmgenet.c
> and found TX completion (__bcmgenet_tx_reclaim()) was freeing skb before
> frags were actually dma unmapped.
> 
> 
> Are you sure this is OK ? bnx2 and bnx2x actually do the reverse (unmap
> all frags before freeing skb)

It does not seem to be much of an issue right now because we put the skb
first, and the frags next in the order we want to transmit them, and we
reclaim in the same order, but it certainly makes more sense to revese
the operation.

> 
> A second problem is the dma_unmap_single() uses tx_cb_ptr->skb->len for
> the length, while it really should be the same thing that was used in
> bcmgenet_xmit_single()

Yes, that is indeed a bug, surprisingly the DMA-API debugging did not
seem to complain about that (should look into why). FWIW, we do not turn
on SG by default, so AFAIR we are not even hitting this path.

Can you submit a formal patch for this and we look into reversing the
mapping of fragments?

Thanks!

> 
> skb_len = skb_headlen(skb) < ETH_ZLEN ? ETH_ZLEN : skb_headlen(skb);
> 
> So maybe something like :
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index d7e01a7..9211b9c7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1197,7 +1197,7 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
>  			dev->stats.tx_bytes += tx_cb_ptr->skb->len;
>  			dma_unmap_single(&dev->dev,
>  					 dma_unmap_addr(tx_cb_ptr, dma_addr),
> -					 tx_cb_ptr->skb->len,
> +					 dma_unmap_len(tx_cb_ptr, dma_len),
>  					 DMA_TO_DEVICE);
>  			bcmgenet_free_cb(tx_cb_ptr);
>  		} else if (dma_unmap_addr(tx_cb_ptr, dma_addr)) {
> @@ -1308,7 +1308,7 @@ static int bcmgenet_xmit_single(struct net_device *dev,
>  	}
>  
>  	dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
> -	dma_unmap_len_set(tx_cb_ptr, dma_len, skb->len);
> +	dma_unmap_len_set(tx_cb_ptr, dma_len, skb_len);
>  	length_status = (skb_len << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
>  			(priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT) |
>  			DMA_TX_APPEND_CRC;
> 
> 


-- 
Florian