netdev - Re: [PATCH 2/3] net: macb: fix dropped RX frames due to a race

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: <79692da1-c097-5fbf-e564-948fc3bc4b3e@microchip.com>
Date:   Wed, 5 Dec 2018 12:38:00 +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 2/3] net: macb: fix dropped RX frames due to a race



On 30.11.2018 20:21, Anssi Hannula wrote:
> Bit RX_USED set to 0 in the address field allows the controller to write
> data to the receive buffer descriptor.
> 
> The driver does not ensure the ctrl field is ready (cleared) when the
> controller sees the RX_USED=0 written by the driver. The ctrl field might
> only be cleared after the controller has already updated it according to
> a newly received frame, causing the frame to be discarded in gem_rx() due
> to unexpected ctrl field contents.
> 
> A message is logged when the above scenario occurs:
> 
>   macb ff0b0000.ethernet eth0: not whole frame pointed by descriptor
> > Fix the issue by ensuring that when the controller sees RX_USED=0 the
> ctrl field is already cleared.
> 
> This issue was observed on a ZynqMP based system.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@...wise.fi>
> Fixes: 4df95131ea80 ("net/macb: change RX path for GEM")
> Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 0bc2aab7be40..430b7a0f5436 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -935,14 +935,19 @@ static void gem_rx_refill(struct macb_queue *queue)
>  
>  			if (entry == bp->rx_ring_size - 1)
>  				paddr |= MACB_BIT(RX_WRAP);
> -			macb_set_addr(bp, desc, paddr);
>  			desc->ctrl = 0;
> +			/* Setting addr clears RX_USED and allows reception,
> +			 * make sure ctrl is cleared first to avoid a race.
> +			 */
> +			dma_wmb();

Same here as in patch 1/1 with regards to memory barrier.

> +			macb_set_addr(bp, desc, paddr);
>  
>  			/* properly align Ethernet header */
>  			skb_reserve(skb, NET_IP_ALIGN);
>  		} else {
> -			desc->addr &= ~MACB_BIT(RX_USED);
>  			desc->ctrl = 0;
> +			dma_wmb();

Ditto

> +			desc->addr &= ~MACB_BIT(RX_USED);
>  		}
>  	}
>  
>