linux-kernel - 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: <CAHk-=wiT5HX6Kp0Qv4ZYK_rkq9t5fZ5zZ7vzvi6pub9kgp=72g@mail.gmail.com>
Date:   Tue, 1 Mar 2022 11:06:45 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     James Bottomley <James.Bottomley@...senpartnership.com>
Cc:     Mike Rapoport <rppt@...nel.org>,
        Christian König <christian.koenig@....com>,
        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>
Subject: Re: [PATCH 2/6] treewide: remove using list iterator after loop body
 as a ptr

On Mon, Feb 28, 2022 at 2:29 PM James Bottomley
<James.Bottomley@...senpartnership.com> wrote:
>
> However, if the desire is really to poison the loop variable then we
> can do
>
> #define list_for_each_entry(pos, head, member)                          \
>         for (pos = list_first_entry(head, typeof(*pos), member);        \
>              !list_entry_is_head(pos, head, member) && ((pos = NULL) == NULL;                   \
>              pos = list_next_entry(pos, member))
>
> Which would at least set pos to NULL when the loop completes.

That would actually have been excellent if we had done that
originally. It would not only avoid the stale and incorrectly typed
head entry left-over turd, it would also have made it very easy to
test for "did I find an entry in the loop".

But I don't much like it in the situation we are now.

Why? Mainly because it basically changes the semantics of the loop
_without_ any warnings about it.  And we don't actually get the
advantage of the nicer semantics, because we can't actually make code
do

        list_for_each_entry(entry, ....) {
                ..
        }
        if (!entry)
                return -ESRCH;
        .. use the entry we found ..

because that would be a disaster for back-porting, plus it would be a
flag-day issue (ie we'd have to change the semantics of the loop at
the same time we change every single user).

So instead of that simple "if (!entry)", we'd effectively have to
continue to use something that still works with the old world order
(ie that "if (list_entry_is_head())" model).

So we couldn't really take _advantage_ of the nicer semantics, and
we'd not even get a warning if somebody does it wrong - the code would
just silently do the wrong thing.

IOW: I don't think you are wrong about that patch: it would solve the
problem that Jakob wants to solve, and it would have absolutely been
much better if we had done this from the beginning. But I think that
in our current situation, it's actually a really fragile solution to
the "don't do that then" problem we have.

              Linus