[Drbd-dev] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Christian König]

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