netdev - Re: [PATCH 3/3] staging: qlge: add comment explaining memory barrier

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]

Date:   Mon, 31 Oct 2022 17:53:08 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     drake@...ketalley.com
Cc:     Manish Chopra <manishc@...vell.com>, GR-Linux-NIC-Dev@...vell.com,
        Coiby Xu <coiby.xu@...il.com>, netdev@...r.kernel.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] staging: qlge: add comment explaining memory barrier

On Mon, Oct 31, 2022 at 10:25:16AM -0400, drake@...ketalley.com wrote:
> From: Drake Talley <drake@...ketalley.com>
> 
> codestyle change that fixes the following report from checkpatch:
> 
> > WARNING: memory barrier without comment
> > #2101: FILE: drivers/staging/qlge/qlge_main.c:2101:
> 
> The added comment identifies the next item from the circular
> buffer (rx_ring->curr_entry) and its handling/unmapping as the two
> operations that must not be reordered.  Based on the kernel
> documentation for memory barriers in circular buffers
> (https://www.kernel.org/doc/Documentation/circular-buffers.txt) and
> the presence of atomic operations in the current context I'm assuming
> this usage of the memory barrier is akin to what is explained in the
> linked doc.
> 
> There are a couple of other uncommented usages of memory barriers in
> the current file.  If this comment is adequate I can add similar
> comments to the others.
> 
> Signed-off-by: Drake Talley <drake@...ketalley.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index c8403dbb5bad..f70390bce6d8 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -2098,6 +2098,12 @@ static int qlge_clean_outbound_rx_ring(struct rx_ring *rx_ring)
>  			     rx_ring->cq_id, prod, rx_ring->cnsmr_idx);
>  
>  		net_rsp = (struct qlge_ob_mac_iocb_rsp *)rx_ring->curr_entry;
> +		/*
> +		 * Ensure that the next item from the ring buffer is loaded
> +		 * before being processed.
> +		 * Adding rmb() prevents the compiler from reordering the read
> +		 * and subsequent handling of the outbound completion pointer.
> +		 */

Which "next item"?

>  		rmb();

>  		switch (net_rsp->opcode) {

So the opcode read is what you want to prevent from reordering?  Where
is the other users of this that could have changed it?

Changes like this are hard to determine if your comments are correct.
We know what a rmb() does, the question that needs to be answered here
is _why_ it is used here.  So try to step back and see if it really is
needed at all.

If it is needed, why?  And go from there on how to document this
properly.

thanks,

greg k-h