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

Linus Torvalds
2022-02-28T19:56:40Z

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

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)

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?

Linus

[-- Attachment #2: p --]
[-- Type: application/octet-stream, Size: 881 bytes --]

diff --git a/include/linux/list.h b/include/linux/list.h
index dd6c2041d09c..bab995596aaa 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -634,10 +634,10 @@ static inline void list_splice_tail_init(struct list_head *list,
* @head: the head for your list.
* @member: the name of the list_head within the struct.
*/
-#define list_for_each_entry(pos, head, member) \
- for (pos = list_first_entry(head, typeof(*pos), member); \
- !list_entry_is_head(pos, head, member); \
- pos = list_next_entry(pos, member))
+#define list_for_each_entry(pos, head, member) \
+ for (typeof(pos) __iter = list_first_entry(head, typeof(*pos), member); \
+ !list_entry_is_head(__iter, head, member) && (((pos)=__iter),1); \
+ __iter = list_next_entry(__iter, member))

/**
* list_for_each_entry_reverse - iterate backwards over list of given type.