netdev - Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body

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:   Tue, 1 Mar 2022 12:36:03 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jakob Koschel <jakobkoschel@...il.com>
Cc:     linux-arch <linux-arch@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergman <arnd@...db.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Mike Rapoport <rppt@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Nathan Chancellor <nathan@...nel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        linux-sgx@...r.kernel.org, drbd-dev@...ts.linbit.com,
        linux-block <linux-block@...r.kernel.org>,
        linux-iio@...r.kernel.org,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        dma <dmaengine@...r.kernel.org>,
        linux1394-devel@...ts.sourceforge.net,
        amd-gfx list <amd-gfx@...ts.freedesktop.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        intel-gfx <intel-gfx@...ts.freedesktop.org>,
        nouveau@...ts.freedesktop.org,
        linux-rdma <linux-rdma@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        intel-wired-lan@...ts.osuosl.org, Netdev <netdev@...r.kernel.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        linux-scsi <linux-scsi@...r.kernel.org>,
        linux-staging@...ts.linux.dev, linux-usb@...r.kernel.org,
        linux-aspeed@...ts.ozlabs.org,
        bcm-kernel-feedback-list@...adcom.com,
        linux-tegra <linux-tegra@...r.kernel.org>,
        linux-mediatek@...ts.infradead.org, KVM list <kvm@...r.kernel.org>,
        CIFS <linux-cifs@...r.kernel.org>,
        samba-technical@...ts.samba.org,
        Linux F2FS Dev Mailing List 
        <linux-f2fs-devel@...ts.sourceforge.net>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        kgdb-bugreport@...ts.sourceforge.net,
        v9fs-developer@...ts.sourceforge.net,
        tipc-discussion@...ts.sourceforge.net, alsa-devel@...a-project.org
Subject: Re: [PATCH 6/6] treewide: remove check of list iterator against head
 past the loop body

So looking at this patch, I really reacted to the fact that quite
often the "use outside the loop" case is all kinds of just plain
unnecessary, but _used_ to be a convenience feature.

I'll just quote the first chunk in it's entirely as an example - not
because I think this chunk is particularly important, but because it's
a good example:

On Mon, Feb 28, 2022 at 3:09 AM Jakob Koschel <jakobkoschel@...il.com> wrote:
>
> diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c
> index 6794e2db1ad5..fc47c107059b 100644
> --- a/arch/arm/mach-mmp/sram.c
> +++ b/arch/arm/mach-mmp/sram.c
> @@ -39,19 +39,22 @@ static LIST_HEAD(sram_bank_list);
>  struct gen_pool *sram_get_gpool(char *pool_name)
>  {
>         struct sram_bank_info *info = NULL;
> +       struct sram_bank_info *tmp;
>
>         if (!pool_name)
>                 return NULL;
>
>         mutex_lock(&sram_lock);
>
> -       list_for_each_entry(info, &sram_bank_list, node)
> -               if (!strcmp(pool_name, info->pool_name))
> +       list_for_each_entry(tmp, &sram_bank_list, node)
> +               if (!strcmp(pool_name, tmp->pool_name)) {
> +                       info = tmp;
>                         break;
> +               }
>
>         mutex_unlock(&sram_lock);
>
> -       if (&info->node == &sram_bank_list)
> +       if (!info)
>                 return NULL;
>
>         return info->gpool;

I realize this was probably at least auto-generated with coccinelle,
but maybe that script could be taught to do avoid the "use after loop"
by simply moving the code _into_ the loop.

IOW, this all would be cleaner and clear written as

        if (!pool_name)
                return NULL;

        mutex_lock(&sram_lock);
        list_for_each_entry(info, &sram_bank_list, node) {
                if (!strcmp(pool_name, info->pool_name)) {
                        mutex_unlock(&sram_lock);
                        return info;
                }
        }
        mutex_unlock(&sram_lock);
        return NULL;

Ta-daa - no use outside the loop, no need for new variables, just a
simple "just do it inside the loop". Yes, we end up having that lock
thing twice, but it looks worth it from a "make the code obvious"
standpoint.

Would it be even cleaner if the locking was done in the caller, and
the loop was some simple helper function? It probably would. But that
would require a bit more smarts than probably a simple coccinelle
script would do.

                Linus