Nouveau - Feb 2022 - [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley <James.Bottomley
at HansenPartnership.com> wrote:
>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 at amd.com> wrote:
>> > > I don't think that using the extra variable makes the
code in any
>> > > way
>> > > more reliable or easier to read.
>> > So I think the next step is to do the attached patch (which
>> > requires
>> > that "-std=gnu11" that was discussed in the original
thread).
>> > 
>> > That will guarantee that the 'pos' parameter of
>> > list_for_each_entry()
>> > is only updated INSIDE the for_each_list_entry() loop, and can
>> > never
>> > point to the (wrongly typed) head entry.
>> > 
>> > And I would actually hope that it should actually cause compiler
>> > warnings about possibly uninitialized variables if people then use
>> > the
>> > 'pos' pointer outside the loop. Except
>> > 
>> >   (a) that code in sgx/encl.c currently initializes 'tmp'
to NULL
>> > for
>> > inexplicable reasons - possibly because it already expected this
>> > behavior
>> > 
>> >   (b) when I remove that NULL initializer, I still don't get a
>> > warning,
>> > because we've disabled -Wno-maybe-uninitialized since it
results in
>> > so
>> > many false positives.
>> > 
>> > Oh well.
>> > 
>> > Anyway, give this patch a look, and at least if it's expanded
to do
>> > "(pos) = NULL" in the entry statement for the for-loop,
it will
>> > avoid the HEAD type confusion that Jakob is working on. And I
think
>> > in a cleaner way than the horrid games he plays.
>> > 
>> > (But it won't avoid possible CPU speculation of such type
>> > confusion. That, in my opinion, is a completely different issue)
>> 
>> Yes, completely agree.
>> 
>> > 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.

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.
>James

-- 
Sincerely yours,
Mike

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 at HansenPartnership.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

On Mon, Feb 28, 2022 at 05:28:58PM -0500, James Bottomley
wrote:
> 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 at HansenPartnership.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.
In C its scope is the rest of the declaration and the entire loop, not
anything after it.  This was the same in C++98 already, btw (but in
pre-standard versions of C++ things were like you remember, yes, and it
was painful).


Segher

On Mon, Feb 28, 2022 at 2:29 PM James Bottomley
<James.Bottomley at hansenpartnership.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