netdev - RE: [PATCH net v2 4/9] net: axienet: add missing memory barriers

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 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]

Date:   Thu, 13 Jan 2022 12:09:41 +0000
From:   Radhey Shyam Pandey <radheys@...inx.com>
To:     Robert Hancock <robert.hancock@...ian.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Michal Simek <michals@...inx.com>,
        "ariane.keller@....ee.ethz.ch" <ariane.keller@....ee.ethz.ch>,
        "daniel@...earbox.net" <daniel@...earbox.net>
Subject: RE: [PATCH net v2 4/9] net: axienet: add missing memory barriers

> -----Original Message-----
> From: Robert Hancock <robert.hancock@...ian.com>
> Sent: Wednesday, January 12, 2022 11:07 PM
> To: netdev@...r.kernel.org
> Cc: Radhey Shyam Pandey <radheys@...inx.com>; davem@...emloft.net;
> kuba@...nel.org; linux-arm-kernel@...ts.infradead.org; Michal Simek
> <michals@...inx.com>; ariane.keller@....ee.ethz.ch; daniel@...earbox.net;
> Robert Hancock <robert.hancock@...ian.com>
> Subject: [PATCH net v2 4/9] net: axienet: add missing memory barriers
> 
> This driver was missing some required memory barriers:
> 
> Use dma_rmb to ensure we see all updates to the descriptor after we see that
> an entry has been completed.
> 
> Use wmb and rmb to avoid stale descriptor status between the TX path and TX
> complete IRQ path.
> 
> Fixes: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
> driver")
> Signed-off-by: Robert Hancock <robert.hancock@...ian.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index f4ae035bed35..de8f85175a6c 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -632,6 +632,8 @@ static int axienet_free_tx_chain(struct net_device
> *ndev, u32 first_bd,
>  		if (nr_bds == -1 && !(status &
> XAXIDMA_BD_STS_COMPLETE_MASK))
>  			break;
> 
> +		/* Ensure we see complete descriptor update */
> +		dma_rmb();
>  		phys = desc_get_phys_addr(lp, cur_p);
>  		dma_unmap_single(ndev->dev.parent, phys,
>  				 (cur_p->cntrl &
> XAXIDMA_BD_CTRL_LENGTH_MASK), @@ -645,8 +647,10 @@ static int
> axienet_free_tx_chain(struct net_device *ndev, u32 first_bd,
>  		cur_p->app1 = 0;
>  		cur_p->app2 = 0;
>  		cur_p->app4 = 0;
> -		cur_p->status = 0;
>  		cur_p->skb = NULL;
> +		/* ensure our transmit path and device don't prematurely see
> status cleared */
> +		wmb();
> +		cur_p->status = 0;

Any reason for moving status initialization down?

> 
>  		if (sizep)
>  			*sizep += status &
> XAXIDMA_BD_STS_ACTUAL_LEN_MASK; @@ -704,6 +708,9 @@ static inline
> int axienet_check_tx_bd_space(struct axienet_local *lp,
>  					    int num_frag)
>  {
>  	struct axidma_bd *cur_p;
> +
> +	/* Ensure we see all descriptor updates from device or TX IRQ path */
> +	rmb();
>  	cur_p = &lp->tx_bd_v[(lp->tx_bd_tail + num_frag) % lp->tx_bd_num];
>  	if (cur_p->status & XAXIDMA_BD_STS_ALL_MASK)
>  		return NETDEV_TX_BUSY;
> @@ -843,6 +850,8 @@ static void axienet_recv(struct net_device *ndev)
> 
>  		tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci;
> 
> +		/* Ensure we see complete descriptor update */
> +		dma_rmb();
>  		phys = desc_get_phys_addr(lp, cur_p);
>  		dma_unmap_single(ndev->dev.parent, phys, lp-
> >max_frm_size,
>  				 DMA_FROM_DEVICE);

Ideally we would also need a write barrier in xmit function just before 
updating tail descriptor.

> --
> 2.31.1