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

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)

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?

                Linus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: p
Type: application/octet-stream
Size: 881 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220228/b8f33f5b/attachment-0001.obj>

On Mon, Feb 28, 2022 at 11:56 AM Linus Torvalds
<torvalds at linux-foundation.org> 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.
Side note: we do need *some* way to do it.

Because if we make that change, and only set it to another pointer
when not the head, then we very much change the semantics of
"list_for_each_head()". The "list was empty" case would now
exit with
'pos' not set at all (or set to NULL if we add that). Or it would be
set to the last entry.

And regardless, that means that all the

        if (pos == head)

kinds of checks after the loop would be fundamentally broken.

Darn. I really hoped for (and naively expected) that we could actually
have the compiler warn about the use-after-loop case. That whole
"compiler will now complain about bad use" was integral to my clever
plan to use the C99 feature of declaring the iterator inside the loop.

But my "clever plan" was apparently some ACME-level Wile E. Coyote
sh*t.

Darn.

                   Linus

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);

So the solution should probably not be to change all those use cases to 
use more temporary variables, but rather to add a list_find_entry(..., 
condition) macro and consistently use that one instead.

Regards,
Christian.
>
>                  Linus

On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds
<torvalds at linux-foundation.org> wrote:
>
> Side note: we do need *some* way to do it.
Ooh.

This patch is a work of art.

And I mean that in the worst possible way.

We can do

        typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than
outside.

And then the compiler will not see some "might be uninitialized", but
the outer 'pos' *will* be uninitialized.

Unless, of course, the outer 'pos' had that pointless explicit
initializer.

Here - can somebody poke holes in this "work of art" patch?

                     Linus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 1927 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220228/92bce193/attachment-0001.bin>