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@xxxxxxx> 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. James