On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley James.Bottomley@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@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