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: <4520e9c5-8871-b281-f621-ac737e64333b@gmail.com>
Date:   Thu, 7 Apr 2022 18:42:17 +0100
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Jakob Koschel <jakobkoschel@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        UNGLinuxDriver@...rochip.com, Ariel Elior <aelior@...vell.com>,
        Manish Chopra <manishc@...vell.com>,
        Martin Habets <habetsm.xilinx@...il.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Jiri Pirko <jiri@...nulli.us>,
        Casper Andersson <casper.casan@...il.com>,
        Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
        Colin Ian King <colin.king@...el.com>,
        Michael Walle <michael@...le.cc>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Arnd Bergmann <arnd@...db.de>,
        Eric Dumazet <edumazet@...gle.com>,
        Di Zhu <zhudi21@...wei.com>, Xu Wang <vulab@...as.ac.cn>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linuxppc-dev@...ts.ozlabs.org, Mike Rapoport <rppt@...nel.org>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [PATCH net-next 11/15] sfc: Remove usage of list iterator for
 list_add() after the loop body

On 07/04/2022 11:28, Jakob Koschel wrote:
> In preparation to limit the scope of a list iterator to the list
> traversal loop, use a dedicated pointer to point to the found element [1].
> 
> Before, the code implicitly used the head when no element was found
> when using &pos->list. Since the new variable is only set if an
> element was found, the list_add() is performed within the loop
> and only done after the loop if it is done on the list head directly.
> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>

The commit message doesn't accurately describe the patch; it states
 that "the list_add() is performed within the loop", which doesn't
 appear to be the case.
Also it seems a bit subtle to use `head` as both the head of the
 list to iterate over and the found entry/gap to insert before; a
 comment explaining that wouldn't go amiss.
(I'd question whether this change is really an improvement in this
 case, where the iterator really does hold the thing we want at the
 end of the search and so there's no if(found) special-casing —
 we're not even abusing the type system, because efx->rss_context
 is of the same type as all the list entries, so ctx really is a
 valid pointer and there shouldn't be any issues with speculative
 accesses or whatever — but it seems Linus has already pronounced
 in favour of the scope limiting, and far be it from me to gainsay
 him.)

-ed

> ---
>  drivers/net/ethernet/sfc/rx_common.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
> index 1b22c7be0088..a8822152ff83 100644
> --- a/drivers/net/ethernet/sfc/rx_common.c
> +++ b/drivers/net/ethernet/sfc/rx_common.c
> @@ -563,8 +563,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>  
>  	/* Search for first gap in the numbering */
>  	list_for_each_entry(ctx, head, list) {
> -		if (ctx->user_id != id)
> +		if (ctx->user_id != id) {
> +			head = &ctx->list;
>  			break;
> +		}
>  		id++;
>  		/* Check for wrap.  If this happens, we have nearly 2^32
>  		 * allocated RSS contexts, which seems unlikely.
> @@ -582,7 +584,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>  
>  	/* Insert the new entry into the gap */
>  	new->user_id = id;
> -	list_add_tail(&new->list, &ctx->list);
> +	list_add_tail(&new->list, head);
>  	return new;
>  }
>  
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ