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

On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:

Am 28.02.22 um 20:56 schrieb Linus Torvalds:

quoted

On Mon, Feb 28, 2022 at 4:19 AM Christian König
[off-list ref] wrote:

quoted

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.

quoted

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.

James