Am 28.02.22 um 20:56 schrieb Linus Torvalds:
On Mon, Feb 28, 2022 at 4:19 AM Christian König
<christian.koe...@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
