Message-Id: <20220304065957.16799-1-xiam0nd.tong@gmail.com>
Date:   Fri,  4 Mar 2022 14:59:57 +0800
From:   Xiaomeng Tong <xiam0nd.tong@...il.com>
To:     daniel.thompson@...aro.org
Cc:     akpm@...ux-foundation.org, alsa-devel@...a-project.org,
        amd-gfx@...ts.freedesktop.org, andriy.shevchenko@...ux.intel.com,
        arnd@...db.de, bcm-kernel-feedback-list@...adcom.com,
        bjohannesmeyer@...il.com, c.giuffrida@...nl,
        christian.koenig@....com, christophe.jaillet@...adoo.fr,
        dan.carpenter@...cle.com, david.laight@...lab.com,
        dmaengine@...r.kernel.org, drbd-dev@...ts.linbit.com,
        dri-devel@...ts.freedesktop.org, gustavo@...eddedor.com,
        h.j.bos@...nl, intel-gfx@...ts.freedesktop.org,
        intel-wired-lan@...ts.osuosl.org, jakobkoschel@...il.com,
        jgg@...pe.ca, keescook@...omium.org,
        kgdb-bugreport@...ts.sourceforge.net, kvm@...r.kernel.org,
        linux-arch@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-aspeed@...ts.ozlabs.org, linux-block@...r.kernel.org,
        linux-cifs@...r.kernel.org, linux-crypto@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-fsdevel@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-pm@...r.kernel.org,
        linux-rdma@...r.kernel.org, linux-scsi@...r.kernel.org,
        linux-sgx@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-tegra@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-wireless@...r.kernel.org,
        linux1394-devel@...ts.sourceforge.net, linux@...musvillemoes.dk,
        linuxppc-dev@...ts.ozlabs.org, nathan@...nel.org,
        netdev@...r.kernel.org, nouveau@...ts.freedesktop.org,
        rppt@...nel.org, samba-technical@...ts.samba.org,
        tglx@...utronix.de, tipc-discussion@...ts.sourceforge.net,
        torvalds@...ux-foundation.org,
        v9fs-developer@...ts.sourceforge.net, xiam0nd.tong@...il.com
Subject: Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

On Thu, 3 Mar 2022 12:18:24 +0000, Daniel Thompson wrote:
> On Thu, Mar 03, 2022 at 03:26:57PM +0800, Xiaomeng Tong wrote:
> > On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
> > > on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
> > > > The problem is the mis-use of iterator outside the loop on exit, and
> > > > the iterator will be the HEAD's container_of pointer which pointers
> > > > to a type-confused struct. Sidenote: The *mis-use* here refers to
> > > > mistakely access to other members of the struct, instead of the
> > > > list_head member which acutally is the valid HEAD.
> > >
> > > The problem is that the HEAD's container_of pointer should never
> > > be calculated at all.
> > > This is what is fundamentally broken about the current definition.
> > 
> > Yes, the rule is "the HEAD's container_of pointer should never be
> > calculated at all outside the loop", but how do you make sure everyone
> > follows this rule?
> 
> Your formulation of the rule is correct: never run container_of() on HEAD
> pointer.

Actually, it is not my rule. My rule is that never access other members
of the struct except for the list_head member after the loop, because
this is a invalid member after loop exit, but valid for the list_head
member which just is HEAD and the lately caculation (&pos->head) seems
harmless.

I have considered the case that the HEAD's container "pos" is layouted
across the max and the min address boundary, which means the address of
HEAD is likely 0x60, and the address of pos is likely 0xffffffe0.
It seems ok to caculate pos with:
((type *)(__mptr - offsetof(type, member)));
and it seems ok to caculate head outside the loop with:
if (&pos->head == &HEAD)
    return NULL;

The only case I can think of with the rule "never run container_of()
on HEAD" must be followed is when the first argument (which is &HEAD)
passing to container_of() is NULL + some offset, it may lead to the
resulting "pos->member" access being a NULL dereference. But maybe
the caller can take the responsibility to check if it is NULL, not
container_of() itself.

Please remind me if i missed somthing, thanks.

> 
> However the rule that is introduced by list_for_each_entry_inside() is
> *not* this rule. The rule it introduces is: never access the iterator
> variable outside the loop.

Sorry for the confusion, indeed, that is two *different* rule.

> 
> Making the iterator NULL on loop exit does follow the rule you proposed
> but using a different technique: do not allow HEAD to be stored in the
> iterator variable after loop exit. This also makes it impossible to run
> container_of() on the HEAD pointer.
> 

It does not. My rule is: never access the iterator variable outside the loop.
The "Making the iterator NULL on loop exit" way still leak the pos with NULL
outside the loop, may lead to a NULL deference.

> 
> > Everyone makes mistakes, but we can eliminate them all from the beginning
> > with the help of compiler which can catch such use-after-loop things.
> 
> Indeed but if we introduce new interfaces then we don't have to worry
> about existing usages and silent regressions. Code will have been
> written knowing the loop can exit with the iterator set to NULL.

Yes, it is more simple and compatible with existing interfaces. Howerver,
you should make every developers to remember that "pos will be set NULL on
loop exit", which is unreasonable and impossible for *every* single person.
Otherwise the mis-use-after-loop will lead to a NULL dereference.
But we can kill this problem by declaring iterator inside the loop and the
complier will catch it if somebody mis-use-after-loop.

> 
> Sure it is still possible for programmers to make mistakes and
> dereference the NULL pointer but C programmers are well training w.r.t.
> NULL pointer checking so such mistakes are much less likely than with
> the current list_for_each_entry() macro. This risk must be offset
> against the way a NULLify approach can lead to more elegant code when we
> are doing a list search.
> 

Yes, the NULLify approach is better than the current list_for_each_entry()
macro, but i stick with that the list_for_each_entry_inside() way is best
and perfect _technically_.

Thus, my idea is *better a finger off than always aching*, let's settle this
damn problem once and for all, with list_for_each_entry_inside().

--
Xiaomeng Tong

Powered by blists - more mailing lists