netdev - Re: [PATCH 3/3] net: macb: add missing barriers when reading buffers

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: <6378cbaf-2c8d-3c22-2d2d-632c32c6195a@microchip.com>
Date:   Wed, 5 Dec 2018 12:37:48 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <anssi.hannula@...wise.fi>, <Nicolas.Ferre@...rochip.com>,
        <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>
Subject: Re: [PATCH 3/3] net: macb: add missing barriers when reading buffers



On 30.11.2018 20:21, Anssi Hannula wrote:
> When reading buffer descriptors on RX or on TX completion, an
> RX_USED/TX_USED bit is checked first to ensure that the descriptor has
> been populated. However, there are no memory barriers to ensure that the
> data protected by the RX_USED/TX_USED bit is up-to-date with respect to
> that bit.
> 
> Fix that by adding DMA read memory barriers on those paths.
> 
> I did not observe any actual issues caused by these being missing,
> though.
> 
> Tested on a ZynqMP based system.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@...wise.fi>
> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
> Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 430b7a0f5436..c93baa8621d5 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -861,6 +861,11 @@ static void macb_tx_interrupt(struct macb_queue *queue)
>  
>  			/* First, update TX stats if needed */
>  			if (skb) {
> +				/* Ensure all of desc is at least as up-to-date
> +				 * as ctrl (TX_USED bit)
> +				 */
> +				dma_rmb();
> +

Is this necessary? Wouldn't previous rmb() take care of this? At this time
data specific to this descriptor was completed. The TX descriptors for next
data to be send is updated under a locked spinlock.

>  				if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
>  					/* skb now belongs to timestamp buffer
>  					 * and will be removed later
> @@ -1000,12 +1005,16 @@ static int gem_rx(struct macb_queue *queue, int budget)
>  		rmb();
>  
>  		rxused = (desc->addr & MACB_BIT(RX_USED)) ? true : false;
> -		addr = macb_get_addr(bp, desc);
> -		ctrl = desc->ctrl;
>  
>  		if (!rxused)
>  			break;
>  
> +		/* Ensure other data is at least as up-to-date as rxused */
> +		dma_rmb();

Same here, wouldn't previous rmb() should do this job?

> +
> +		addr = macb_get_addr(bp, desc);
> +		ctrl = desc->ctrl;
> +
>  		queue->rx_tail++;
>  		count++;
>  
> @@ -1180,11 +1189,14 @@ static int macb_rx(struct macb_queue *queue, int budget)
>  		/* Make hw descriptor updates visible to CPU */
>  		rmb();
>  
> -		ctrl = desc->ctrl;
> -
>  		if (!(desc->addr & MACB_BIT(RX_USED)))
>  			break;
>  
> +		/* Ensure other data is at least as up-to-date as addr */
> +		dma_rmb();

Ditto

> +
> +		ctrl = desc->ctrl;
> +
>  		if (ctrl & MACB_BIT(RX_SOF)) {
>  			if (first_frag != -1)
>  				discard_partial_frame(queue, first_frag, tail);
>