netdev - Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

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



Am 28.02.22 um 21:42 schrieb James Bottomley:
> On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
>>> On Mon, Feb 28, 2022 at 4:19 AM Christian König
>>> <christian.koenig@....com> wrote:
>>> [SNIP]
>>> Anybody have any ideas?
>> I think we should look at the use cases why code is touching (pos)
>> after the loop.
>>
>> Just from skimming over the patches to change this and experience
>> with the drivers/subsystems I help to maintain I think the primary
>> pattern looks something like this:
>>
>> list_for_each_entry(entry, head, member) {
>>       if (some_condition_checking(entry))
>>           break;
>> }
>> do_something_with(entry);
>
> Actually, we usually have a check to see if the loop found anything,
> but in that case it should something like
>
> if (list_entry_is_head(entry, head, member)) {
>      return with error;
> }
> do_somethin_with(entry);
>
> Suffice?  The list_entry_is_head() macro is designed to cope with the
> bogus entry on head problem.

That will work and is also what people already do.

The key problem is that we let people do the same thing over and over 
again with slightly different implementations.

Out in the wild I've seen at least using a separate variable, using a 
bool to indicate that something was found and just assuming that the 
list has an entry.

The last case is bogus and basically what can break badly.

If we would have an unified macro which search for an entry combined 
with automated reporting on patches to use that macro I think the 
potential to introduce such issues will already go down massively 
without auditing tons of existing code.

Regards,
Christian.

>
> James
>
>