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: <73fa82a20910c06784be2352a655acc59e9942ea.camel@HansenPartnership.com>
Date:   Mon, 28 Feb 2022 17:28:58 -0500
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Mike Rapoport <rppt@...nel.org>,
        Christian König <christian.koenig@....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>
Subject: Re: [PATCH 2/6] treewide: remove using list iterator after loop
 body as a ptr

On Mon, 2022-02-28 at 23:59 +0200, Mike Rapoport wrote:
> 
> On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley <
> James.Bottomley@...senPartnership.com> wrote:
> > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
[...]
> > > > I do wish we could actually poison the 'pos' value after the
> > > > loop somehow - but clearly the "might be uninitialized" I was
> > > > hoping for isn't the way to do it.
> > > > 
> > > > 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.
> 
> Won't suffice because the end goal of this work is to limit scope of
> entry only to loop. Hence the need for additional variable.

Well, yes, but my objection is more to the size of churn than the
desire to do loop local.  I'm not even sure loop local is possible,
because it's always annoyed me that for (int i = 0; ...  in C++ defines
i in the outer scope not the loop scope, which is why I never use it.

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.

> Besides, there are no guarantees that people won't
> do_something_with(entry) without the check or won't compare entry to
> NULL to check if the loop finished with break or not.

I get the wider goal, but we have to patch the problem cases now and a
simple one-liner is better than a larger patch that may or may not work
if we ever achieve the local definition or value poisoning idea.  I'm
also fairly certain coccinelle can come up with a use without checking
for loop completion semantic patch which we can add to 0day.

James