netdev - Re: [PATCH net-next v6 1/2] net: axienet: Be more careful about updating tx_bd

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] [day] [month] [year] [list]

Date:   Wed, 11 May 2022 20:40:38 +0000
From:   Robert Hancock <robert.hancock@...ian.com>
To:     "kuba@...nel.org" <kuba@...nel.org>
CC:     "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        "radhey.shyam.pandey@...inx.com" <radhey.shyam.pandey@...inx.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH net-next v6 1/2] net: axienet: Be more careful about
 updating tx_bd_tail

On Wed, 2022-05-11 at 13:00 -0700, Jakub Kicinski wrote:
> On Wed, 11 May 2022 12:44:31 -0600 Robert Hancock wrote:
> > The axienet_start_xmit function was updating the tx_bd_tail variable
> > multiple times, with potential rollbacks on error or invalid
> > intermediate positions, even though this variable is also used in the
> > TX completion path. Use READ_ONCE and WRITE_ONCE to make this update
> > more atomic, and move the write before the MMIO write to start the
> > transfer, so it is protected by that implicit write barrier.
> > 
> > Signed-off-by: Robert Hancock <robert.hancock@...ian.com>
> > ---
> >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 23 +++++++++++--------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > index d6fc3f7acdf0..2f39eb4de249 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > @@ -807,12 +807,15 @@ axienet_start_xmit(struct sk_buff *skb, struct
> > net_device *ndev)
> >  	u32 csum_index_off;
> >  	skb_frag_t *frag;
> >  	dma_addr_t tail_p, phys;
> > +	u32 orig_tail_ptr, new_tail_ptr;
> >  	struct axienet_local *lp = netdev_priv(ndev);
> >  	struct axidma_bd *cur_p;
> > -	u32 orig_tail_ptr = lp->tx_bd_tail;
> > +
> > +	orig_tail_ptr = READ_ONCE(lp->tx_bd_tail);
> 
> This one does not need READ_ONCE().
> 
> We only need to protect reads and writes which may race with each other.
> This read can't race with any write. We need WRITE_ONCE() in
> axienet_start_xmit() and READ_ONCE() in xienet_check_tx_bd_space().

Makes sense, can fix that up.

> 
> BTW I'm slightly murky on what the rmb() in xienet_check_tx_bd_space()
> does. Memory barrier is a fence, not a flush, I don't see what two
> accesses that rmb() is separating.

I believe the idea is to ensure that we're seeing a complete descriptor update
from the hardware (i.e. what dma_rmb does), and also that the last write to
tx_bd_tail will be visible (basically pairing with the implicit write barrier
from the IO write in axienet_start_xmit)?

> 
> > +	new_tail_ptr = orig_tail_ptr;
> >  
> >  	num_frag = skb_shinfo(skb)->nr_frags;
> > -	cur_p = &lp->tx_bd_v[lp->tx_bd_tail];
> > +	cur_p = &lp->tx_bd_v[orig_tail_ptr];
> >  
> >  	if (axienet_check_tx_bd_space(lp, num_frag + 1)) {
> >  		/* Should not happen as last start_xmit call should have
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com