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

Message-ID: <20220511130028.3e2edb35@kernel.org>
Date:   Wed, 11 May 2022 13:00:28 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Robert Hancock <robert.hancock@...ian.com>
Cc:     netdev@...r.kernel.org, radhey.shyam.pandey@...inx.com,
        davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
        michal.simek@...inx.com, 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, 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().

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.

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