Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 11:56 AM Linus Torvalds torvalds@linux-foundation.org wrote:
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.
Side note: we do need *some* way to do it.
Because if we make that change, and only set it to another pointer when not the head, then we very much change the semantics of "list_for_each_head()". The "list was empty" case would now exit with 'pos' not set at all (or set to NULL if we add that). Or it would be set to the last entry.
And regardless, that means that all the
if (pos == head)
kinds of checks after the loop would be fundamentally broken.
Darn. I really hoped for (and naively expected) that we could actually have the compiler warn about the use-after-loop case. That whole "compiler will now complain about bad use" was integral to my clever plan to use the C99 feature of declaring the iterator inside the loop.
But my "clever plan" was apparently some ACME-level Wile E. Coyote sh*t.
Darn.
Linus
On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds torvalds@linux-foundation.org wrote:
Side note: we do need *some* way to do it.
Ooh.
This patch is a work of art.
And I mean that in the worst possible way.
We can do
typeof(pos) pos
in the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
And then the compiler will not see some "might be uninitialized", but the outer 'pos' *will* be uninitialized.
Unless, of course, the outer 'pos' had that pointless explicit initializer.
Here - can somebody poke holes in this "work of art" patch?
Linus
On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds torvalds@linux-foundation.org wrote:
We can do
typeof(pos) posin the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
The thing that makes me throw up in my mouth a bit is that in that
typeof(pos) pos
the first 'pos' (that we use for just the typeof) is that outer-level 'pos', IOW it's a *different* 'pos' than the second 'pos' in that same declaration that declares the inner level shadowing new 'pos' variable.
If I was a compiler person, I would say "Linus, that thing is too ugly to live", and I would hate it. I'm just hoping that even compiler people say "that's *so* ugly it's almost beautiful".
Because it does seem to work. It's not pretty, but hey, it's not like our headers are really ever be winning any beauty contests...
Linus
On Mon, Feb 28, 2022 at 12:14:44PM -0800, Linus Torvalds wrote:
On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds torvalds@linux-foundation.org wrote:
We can do
typeof(pos) posin the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
The thing that makes me throw up in my mouth a bit is that in that
typeof(pos) posthe first 'pos' (that we use for just the typeof) is that outer-level 'pos', IOW it's a *different* 'pos' than the second 'pos' in that same declaration that declares the inner level shadowing new 'pos' variable.
The new "pos" has not yet been declared, so this has to refer to the outer "pos", it cannot be the inner one. Because it hasn't been declared yet :-)
Compare this to typeof (pos) pos = pos; where that last "pos" *does* refer to the newly declared one: that declaration has already been done! (So this code is UB btw, 6.3.2.1/2).
If I was a compiler person, I would say "Linus, that thing is too ugly to live", and I would hate it. I'm just hoping that even compiler people say "that's *so* ugly it's almost beautiful".
It is perfectly well-defined. Well, it would be good if we (GCC) would document it does work, and if someone tested it on LLVM as well. But it is really hard to implement it to *not* work :-)
Because it does seem to work. It's not pretty, but hey, it's not like our headers are really ever be winning any beauty contests...
It is very pretty! Needs a comment though :-)
Segher
On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote:
We can do
typeof(pos) posin the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs.
+#define list_for_each_entry(pos, head, member) \
- for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); \
!list_entry_is_head(pos, head, member); \ pos = list_next_entry(pos, member))
On Mon, 2022-02-28 at 20:16 +0000, Matthew Wilcox wrote:
On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote:
We can do
typeof(pos) posin the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs.
I was just going to say the same thing...
If we're willing to change the API for the macro, we could do
list_for_each_entry(type, pos, head, member)
and then actually take advantage of -Wshadow?
johannes
On Mon, Feb 28, 2022 at 12:29 PM Johannes Berg johannes@sipsolutions.net wrote:
If we're willing to change the API for the macro, we could do
list_for_each_entry(type, pos, head, member)
and then actually take advantage of -Wshadow?
See my reply to Willy. There is no way -Wshadow will ever happen.
I considered that (type, pos, head, member) kind of thing, to the point of trying it for one file, but it ends up as horrendous syntax. It turns out that declaring the type separately really helps, and avoids crazy long lines among other things.
It would be unacceptable for another reason too - the amount of churn would just be immense. Every single use of that macro (and related macros) would change, even the ones that really don't need it or want it (ie the good kinds that already only use the variable inside the loop).
So "typeof(pos) pos" may be ugly - but it's a very localized ugly.
Linus
On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox willy@infradead.org wrote:
Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs.
Oh, we already can never use -Wshadow regardless of things like this. That bridge hasn't just been burned, it never existed in the first place.
The whole '-Wshadow' thing simply cannot work with local variables in macros - something that we've used since day 1.
Try this (as a "p.c" file):
#define min(a,b) ({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ __a < __b ? __a : __b; })
int min3(int a, int b, int c) { return min(a,min(b,c)); }
and now do "gcc -O2 -S t.c".
Then try it with -Wshadow.
In other words, -Wshadow is simply not acceptable. Never has been, never will be, and that has nothing to do with the
typeof(pos) pos
kind of thing.
Your argument just isn't an argument.
Linus
On Mon, Feb 28, 2022 at 12:37:15PM -0800, Linus Torvalds wrote:
On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox willy@infradead.org wrote:
Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs.
Oh, we already can never use -Wshadow regardless of things like this. That bridge hasn't just been burned, it never existed in the first place.
The whole '-Wshadow' thing simply cannot work with local variables in macros - something that we've used since day 1.
Try this (as a "p.c" file):
#define min(a,b) ({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ __a < __b ? __a : __b; }) int min3(int a, int b, int c) { return min(a,min(b,c)); }and now do "gcc -O2 -S t.c".
Then try it with -Wshadow.
#define ___PASTE(a, b) a##b #define __PASTE(a, b) ___PASTE(a, b) #define _min(a, b, u) ({ \ typeof(a) __PASTE(__a,u) = (a); \ typeof(b) __PASTE(__b,u) = (b); \ __PASTE(__a,u) < __PASTE(__b,u) ? __PASTE(__a,u) : __PASTE(__b,u); })
#define min(a, b) _min(a, b, __COUNTER__)
int min3(int a, int b, int c) { return min(a,min(b,c)); }
(probably there's a more elegant way to do this)
On Mon, Feb 28, 2022 at 3:26 PM Matthew Wilcox willy@infradead.org wrote:
#define ___PASTE(a, b) a##b #define __PASTE(a, b) ___PASTE(a, b) #define _min(a, b, u) ({ \
Yeah, except that's ugly beyond belief, plus it's literally not what we do in the kernel.
Really. The "-Wshadow doesn't work on the kernel" is not some new issue, because you have to do completely insane things to the source code to enable it.
Just compare your uglier-than-sin version to my straightforward one. One does the usual and obvious "use a private variable to avoid the classic multi-use of a macro argument". And the other one is an abomination.
Linus
On Mon, Feb 28, 2022 at 4:45 PM Linus Torvalds torvalds@linux-foundation.org wrote:
Yeah, except that's ugly beyond belief, plus it's literally not what we do in the kernel.
(Of course, I probably shouldn't have used 'min()' as an example, because that is actually one of the few places where we do exactly that, using our __UNIQUE_ID() macros. Exactly because people _have_ tried to do -Wshadow when doing W=2).
Linus
On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote:
Really. The "-Wshadow doesn't work on the kernel" is not some new issue, because you have to do completely insane things to the source code to enable it.
The first big glitch with -Wshadow was with shadowed global variables. GCC 4.8 fixed that, but it still yells about shadowed functions. What _almost_ works is -Wshadow=local. At first glace, all the warnings look solvable, but then one will eventually discover __wait_event() and associated macros that mix when and how deeply it intentionally shadows variables. :)
Another way to try to catch misused shadow variables is -Wunused-but-set-varible, but it, too, has tons of false positives.
I tried to capture some of the rationale and research here: https://github.com/KSPP/linux/issues/152
On Tue, Mar 1, 2022 at 10:14 AM Kees Cook keescook@chromium.org wrote:
The first big glitch with -Wshadow was with shadowed global variables. GCC 4.8 fixed that, but it still yells about shadowed functions. What _almost_ works is -Wshadow=local.
Heh. Yeah, I just have long memories of "-Wshadow was a disaster". You looked into the details.
Another way to try to catch misused shadow variables is -Wunused-but-set-varible, but it, too, has tons of false positives.
That on the face of it should be an easy warning to get technically right for a compiler.
So I assume the "false positives" are simply because we end up having various variables that really don't end up being used - and "intentionally" so).
Or rather, they might only be used under some config option - perhaps the use is even syntactically there and parsed, but the compiler notices that it's turned off under some
if (IS_ENABLED(..))
option? Because yeah, we have a lot of those.
I think that's a common theme with a lot of compiler warnings: on the face of it they sound "obviously sane" and nobody should ever write code like that.
A conditional that is always true? Sounds idiotic, and sounds like a reasonable thing for a compiler to warn about, since why would you have a conditional in the first place for that?
But then you realize that maybe the conditional is a build config option, and "always true" suddenly makes sense. Or it's a test for something that is always true on _that_architecture_ but not in some general sense (ie testing "sizeof()"). Or it's a purely syntactic conditional, like "do { } while (0)".
It's why I'm often so down on a lot of the odd warnings that are hiding under W=1 and friends. They all may make sense in the trivial case ("That is insane") but then in the end they happen for sane code.
And yeah, -Wshadow has had tons of history with macro nesting, and just being badly done in the first place (eg "strlen" can be a perfectly fine local variable).
That said, maybe people could ask the gcc and clan people for a way to _mark_ the places where we expect to validly see shadowing. For example, that "local variable in a macro expression statement" thing is absolutely horrendous to fix with preprocessor tricks to try to make for unique identifiers.
But I think it would be much more syntactically reasonable to add (for example) a "shadow" attribute to such a variable exactly to tell the compiler "yeah, yeah, I know this identifier could shadow an outer one" and turn it off that way.
Linus
On Tue, Mar 01, 2022 at 10:14:07AM -0800, Kees Cook wrote:
On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote:
Really. The "-Wshadow doesn't work on the kernel" is not some new issue, because you have to do completely insane things to the source code to enable it.
The first big glitch with -Wshadow was with shadowed global variables. GCC 4.8 fixed that, but it still yells about shadowed functions. What _almost_ works is -Wshadow=local. At first glace, all the warnings look solvable, but then one will eventually discover __wait_event() and associated macros that mix when and how deeply it intentionally shadows variables. :)
Well, that's just disgusting. Macros fundamentally shouldn't be referring to things that aren't in their arguments. The first step to cleaning this up is ...
I'll take a look at the rest of cleaning this up soon.
From 28ffe35d56223d4242b915832299e5acc926737e Mon Sep 17 00:00:00 2001
From: "Matthew Wilcox (Oracle)" willy@infradead.org Date: Tue, 1 Mar 2022 13:47:07 -0500 Subject: [PATCH] wait: Parameterize the return variable to ___wait_event()
Macros should not refer to variables which aren't in their arguments. Pass the name from its callers.
Signed-off-by: Matthew Wilcox (Oracle) willy@infradead.org --- include/linux/swait.h | 12 ++++++------ include/linux/wait.h | 32 ++++++++++++++++---------------- include/linux/wait_bit.h | 4 ++-- 3 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/include/linux/swait.h b/include/linux/swait.h index 6a8c22b8c2a5..5e8e9b13be2d 100644 --- a/include/linux/swait.h +++ b/include/linux/swait.h @@ -191,14 +191,14 @@ do { \ } while (0)
#define __swait_event_timeout(wq, condition, timeout) \ - ___swait_event(wq, ___wait_cond_timeout(condition), \ + ___swait_event(wq, ___wait_cond_timeout(condition, __ret), \ TASK_UNINTERRUPTIBLE, timeout, \ __ret = schedule_timeout(__ret))
#define swait_event_timeout_exclusive(wq, condition, timeout) \ ({ \ long __ret = timeout; \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __swait_event_timeout(wq, condition, timeout); \ __ret; \ }) @@ -216,14 +216,14 @@ do { \ })
#define __swait_event_interruptible_timeout(wq, condition, timeout) \ - ___swait_event(wq, ___wait_cond_timeout(condition), \ + ___swait_event(wq, ___wait_cond_timeout(condition, __ret), \ TASK_INTERRUPTIBLE, timeout, \ __ret = schedule_timeout(__ret))
#define swait_event_interruptible_timeout_exclusive(wq, condition, timeout)\ ({ \ long __ret = timeout; \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __swait_event_interruptible_timeout(wq, \ condition, timeout); \ __ret; \ @@ -252,7 +252,7 @@ do { \ } while (0)
#define __swait_event_idle_timeout(wq, condition, timeout) \ - ___swait_event(wq, ___wait_cond_timeout(condition), \ + ___swait_event(wq, ___wait_cond_timeout(condition, __ret), \ TASK_IDLE, timeout, \ __ret = schedule_timeout(__ret))
@@ -278,7 +278,7 @@ do { \ #define swait_event_idle_timeout_exclusive(wq, condition, timeout) \ ({ \ long __ret = timeout; \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __swait_event_idle_timeout(wq, \ condition, timeout); \ __ret; \ diff --git a/include/linux/wait.h b/include/linux/wait.h index 851e07da2583..890cce3c0f2e 100644 --- a/include/linux/wait.h +++ b/include/linux/wait.h @@ -271,7 +271,7 @@ static inline void wake_up_pollfree(struct wait_queue_head *wq_head) __wake_up_pollfree(wq_head); }
-#define ___wait_cond_timeout(condition) \ +#define ___wait_cond_timeout(condition, __ret) \ ({ \ bool __cond = (condition); \ if (__cond && !__ret) \ @@ -386,7 +386,7 @@ do { \ })
#define __wait_event_timeout(wq_head, condition, timeout) \ - ___wait_event(wq_head, ___wait_cond_timeout(condition), \ + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \ TASK_UNINTERRUPTIBLE, 0, timeout, \ __ret = schedule_timeout(__ret))
@@ -413,13 +413,13 @@ do { \ ({ \ long __ret = timeout; \ might_sleep(); \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_timeout(wq_head, condition, timeout); \ __ret; \ })
#define __wait_event_freezable_timeout(wq_head, condition, timeout) \ - ___wait_event(wq_head, ___wait_cond_timeout(condition), \ + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \ TASK_INTERRUPTIBLE, 0, timeout, \ __ret = freezable_schedule_timeout(__ret))
@@ -431,7 +431,7 @@ do { \ ({ \ long __ret = timeout; \ might_sleep(); \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_freezable_timeout(wq_head, condition, timeout); \ __ret; \ }) @@ -503,7 +503,7 @@ do { \ })
#define __wait_event_interruptible_timeout(wq_head, condition, timeout) \ - ___wait_event(wq_head, ___wait_cond_timeout(condition), \ + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \ TASK_INTERRUPTIBLE, 0, timeout, \ __ret = schedule_timeout(__ret))
@@ -531,7 +531,7 @@ do { \ ({ \ long __ret = timeout; \ might_sleep(); \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_interruptible_timeout(wq_head, \ condition, timeout); \ __ret; \ @@ -698,7 +698,7 @@ do { \ } while (0)
#define __wait_event_idle_timeout(wq_head, condition, timeout) \ - ___wait_event(wq_head, ___wait_cond_timeout(condition), \ + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \ TASK_IDLE, 0, timeout, \ __ret = schedule_timeout(__ret))
@@ -725,13 +725,13 @@ do { \ ({ \ long __ret = timeout; \ might_sleep(); \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_idle_timeout(wq_head, condition, timeout); \ __ret; \ })
#define __wait_event_idle_exclusive_timeout(wq_head, condition, timeout) \ - ___wait_event(wq_head, ___wait_cond_timeout(condition), \ + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \ TASK_IDLE, 1, timeout, \ __ret = schedule_timeout(__ret))
@@ -762,7 +762,7 @@ do { \ ({ \ long __ret = timeout; \ might_sleep(); \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_idle_exclusive_timeout(wq_head, condition, timeout);\ __ret; \ }) @@ -932,7 +932,7 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *); })
#define __wait_event_killable_timeout(wq_head, condition, timeout) \ - ___wait_event(wq_head, ___wait_cond_timeout(condition), \ + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \ TASK_KILLABLE, 0, timeout, \ __ret = schedule_timeout(__ret))
@@ -962,7 +962,7 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *); ({ \ long __ret = timeout; \ might_sleep(); \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_killable_timeout(wq_head, \ condition, timeout); \ __ret; \ @@ -1107,7 +1107,7 @@ do { \ })
#define __wait_event_lock_irq_timeout(wq_head, condition, lock, timeout, state) \ - ___wait_event(wq_head, ___wait_cond_timeout(condition), \ + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \ state, 0, timeout, \ spin_unlock_irq(&lock); \ __ret = schedule_timeout(__ret); \ @@ -1141,7 +1141,7 @@ do { \ timeout) \ ({ \ long __ret = timeout; \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_lock_irq_timeout( \ wq_head, condition, lock, timeout, \ TASK_INTERRUPTIBLE); \ @@ -1151,7 +1151,7 @@ do { \ #define wait_event_lock_irq_timeout(wq_head, condition, lock, timeout) \ ({ \ long __ret = timeout; \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_event_lock_irq_timeout( \ wq_head, condition, lock, timeout, \ TASK_UNINTERRUPTIBLE); \ diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h index 7dec36aecbd9..227e6a20a978 100644 --- a/include/linux/wait_bit.h +++ b/include/linux/wait_bit.h @@ -292,7 +292,7 @@ do { \ })
#define __wait_var_event_timeout(var, condition, timeout) \ - ___wait_var_event(var, ___wait_cond_timeout(condition), \ + ___wait_var_event(var, ___wait_cond_timeout(condition, __ret), \ TASK_UNINTERRUPTIBLE, 0, timeout, \ __ret = schedule_timeout(__ret))
@@ -300,7 +300,7 @@ do { \ ({ \ long __ret = timeout; \ might_sleep(); \ - if (!___wait_cond_timeout(condition)) \ + if (!___wait_cond_timeout(condition, __ret)) \ __ret = __wait_var_event_timeout(var, condition, timeout); \ __ret; \ })
From: Matthew Wilcox
Sent: 28 February 2022 20:16
On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote:
We can do
typeof(pos) posin the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs.
+#define list_for_each_entry(pos, head, member) \
- for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); \
!list_entry_is_head(pos, head, member); \ pos = list_next_entry(pos, member))
Actually can't you use 'pos' to temporarily hold the address of 'member'. Something like: for (pos = (void *)head; \ pos ? ((pos = (void *)pos - offsetof(member)), 1) : 0; \ pos = (void *)pos->next) So that 'pos' is NULL if the loop terminates. No pointers outside structures are generated. Probably need to kill list_entry_is_head() - or it just checks for NULL.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 28. Feb 2022, at 21:10, Linus Torvalds torvalds@linux-foundation.org wrote:
On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds torvalds@linux-foundation.org wrote:
Side note: we do need *some* way to do it.
Ooh.
This patch is a work of art.
And I mean that in the worst possible way.
We can do
typeof(pos) posin the 'for ()' loop, and never use __iter at all.
That means that inside the for-loop, we use a _different_ 'pos' than outside.
And then the compiler will not see some "might be uninitialized", but the outer 'pos' *will* be uninitialized.
Unless, of course, the outer 'pos' had that pointless explicit initializer.
The goal of this is to get compiler warnings right? This would indeed be great.
Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()). I assumed it is better to fix those cases first and then have a simple coccinelle script changing the macro + moving the iterator into the scope of the macro.
Here - can somebody poke holes in this "work of art" patch?
With this you are no longer able to set the 'outer' pos within the list iterator loop body or am I missing something? Like this it stays uninitialized but you'll probably want to set it from within the loop.
You would then yet again need a variable with another name to use after the loop.
I fail to see how this will make most of the changes in this patch obsolete (if that was the intention).
Linus<patch.diff>
- Jakob
On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel jakobkoschel@gmail.com wrote:
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered using some automated checker tool, but I don't think that works from a long-term maintenance perspective.
So if we have the basic rule being "don't use the loop iterator after the loop has finished, because it can cause all kinds of subtle issues", then in _addition_ to fixing the existing code paths that have this issue, I really would want to (a) get a compiler warning for future cases and (b) make it not actually _work_ for future cases.
Because otherwise it will just happen again.
Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already have a patch for due to your automated tool. There were certainly more than a handful, but it didn't look _too_ bad to just make the rule be "don't use the iterator after the loop".
Of course, that's just based on that patch of yours. Maybe there are a ton of other cases that your patch didn't change, because they didn't match your trigger case, so I may just be overly optimistic here.
But basically to _me_, the important part is that the end result is maintainable longer-term. I'm more than happy to have a one-time patch to fix a lot of dubious cases if we can then have clean rules going forward.
I assumed it is better to fix those cases first and then have a simple coccinelle script changing the macro + moving the iterator into the scope of the macro.
So that had been another plan of mine, until I actually looked at changing the macro. In the one case I looked at, it was ugly beyond belief.
It turns out that just syntactically, it's really nice to give the type of the iterator from outside the way we do now. Yeah, it may be a bit odd, and maybe it's partly because I'm so used to the "list_for_each_list_entry()" syntax, but moving the type into the loop construct really made it nasty - either one very complex line, or having to split it over two lines which was even worse.
Maybe the place I looked at just happened to have a long typename, but it's basically always going to be a struct, so it's never a _simple_ type. And it just looked very odd adn unnatural to have the type as one of the "arguments" to that list_for_each_entry() macro.
So yes, initially my idea had been to just move the iterator entirely inside the macro. But specifying the type got so ugly that I think that
typeof (pos) pos
trick inside the macro really ends up giving us the best of all worlds:
(a) let's us keep the existing syntax and code for all the nice cases that did everything inside the loop anyway
(b) gives us a nice warning for any normal use-after-loop case (unless you explicitly initialized it like that sgx_mmu_notifier_release() function did for no good reason
(c) also guarantees that even if you don't get a warning, non-converted (or newly written) bad code won't actually _work_
so you end up getting the new rules without any ambiguity or mistaken
With this you are no longer able to set the 'outer' pos within the list iterator loop body or am I missing something?
Correct. Any assignment inside the loop will be entirely just to the local loop case. So any "break;" out of the loop will have to set another variable - like your updated patch did.
I fail to see how this will make most of the changes in this patch obsolete (if that was the intention).
I hope my explanation above clarifies my thinking: I do not dislike your patch, and in fact your patch is indeed required to make the new semantics work.
What I disliked was always the maintainability of your patch - making the rules be something that isn't actually visible in the source code, and letting the old semantics still work as well as they ever did, and having to basically run some verification pass to find bad users.
(I also disliked your original patch that mixed up the "CPU speculation type safety" with the actual non-speculative problems, but that was another issue).
Linus
On Mon, 28 Feb 2022 16:41:04 -0800 Linus Torvalds wrote:
So yes, initially my idea had been to just move the iterator entirely inside the macro. But specifying the type got so ugly that I think that
typeof (pos) postrick inside the macro really ends up giving us the best of all worlds:
(a) let's us keep the existing syntax and code for all the nice cases that did everything inside the loop anyway
(b) gives us a nice warning for any normal use-after-loop case (unless you explicitly initialized it like that sgx_mmu_notifier_release() function did for no good reason
(c) also guarantees that even if you don't get a warning, non-converted (or newly written) bad code won't actually _work_
so you end up getting the new rules without any ambiguity or mistaken
I presume the goal is that we can do this without changing existing code? Otherwise actually moving the iterator into the loop body would be an option, by creating a different hidden variable:
#define list_iter(head) \ for (struct list head *_l = (head)->next; _l != (head); _l = _l->next)
#define list_iter_entry(var, member) \ list_entry(_l, typeof(*var), member)
list_iter(&p->a_head) { struct entry *e = list_iter_entry(e, a_member);
/* use e->... */ }
Or we can slide into soft insanity and exploit one of Kees'es tricks to encode the type of the entries "next to" the head:
#define LIST_HEAD_MEM(name, type) \ union { \ struct list_head name; \ type *name ## _entry; \ }
struct entry { struct list_head a_member; };
struct parent { LIST_HEAD_MEM(a_head, struct entry); };
#define list_for_each_magic(pos, head, member) \ for (typeof(**(head ## _entry)) *pos = list_first_entry(head, typeof(**(head ## _entry)), member); \ &pos->member != (head); \ pos = list_next_entry(pos, member))
list_for_each_magic(e, &p->a_head, a_member) { /* use e->... */ }
I'll show myself out...
On 1. Mar 2022, at 01:41, Linus Torvalds torvalds@linux-foundation.org wrote:
On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel jakobkoschel@gmail.com wrote:
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered using some automated checker tool, but I don't think that works from a long-term maintenance perspective.
So if we have the basic rule being "don't use the loop iterator after the loop has finished, because it can cause all kinds of subtle issues", then in _addition_ to fixing the existing code paths that have this issue, I really would want to (a) get a compiler warning for future cases and (b) make it not actually _work_ for future cases.
Because otherwise it will just happen again.
Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already have a patch for due to your automated tool. There were certainly more than a handful, but it didn't look _too_ bad to just make the rule be "don't use the iterator after the loop".
Of course, that's just based on that patch of yours. Maybe there are a ton of other cases that your patch didn't change, because they didn't match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing in total. I'll now finish all of them and then split them by submodules as Greg suggested and repost a patch set per submodule. Sounds good?
But basically to _me_, the important part is that the end result is maintainable longer-term. I'm more than happy to have a one-time patch to fix a lot of dubious cases if we can then have clean rules going forward.
I assumed it is better to fix those cases first and then have a simple coccinelle script changing the macro + moving the iterator into the scope of the macro.
So that had been another plan of mine, until I actually looked at changing the macro. In the one case I looked at, it was ugly beyond belief.
It turns out that just syntactically, it's really nice to give the type of the iterator from outside the way we do now. Yeah, it may be a bit odd, and maybe it's partly because I'm so used to the "list_for_each_list_entry()" syntax, but moving the type into the loop construct really made it nasty - either one very complex line, or having to split it over two lines which was even worse.
Maybe the place I looked at just happened to have a long typename, but it's basically always going to be a struct, so it's never a _simple_ type. And it just looked very odd adn unnatural to have the type as one of the "arguments" to that list_for_each_entry() macro.
So yes, initially my idea had been to just move the iterator entirely inside the macro. But specifying the type got so ugly that I think that
typeof (pos) postrick inside the macro really ends up giving us the best of all worlds:
(a) let's us keep the existing syntax and code for all the nice cases that did everything inside the loop anyway
(b) gives us a nice warning for any normal use-after-loop case (unless you explicitly initialized it like that sgx_mmu_notifier_release() function did for no good reason
(c) also guarantees that even if you don't get a warning, non-converted (or newly written) bad code won't actually _work_
so you end up getting the new rules without any ambiguity or mistaken
With this you are no longer able to set the 'outer' pos within the list iterator loop body or am I missing something?
Correct. Any assignment inside the loop will be entirely just to the local loop case. So any "break;" out of the loop will have to set another variable - like your updated patch did.
I fail to see how this will make most of the changes in this patch obsolete (if that was the intention).
I hope my explanation above clarifies my thinking: I do not dislike your patch, and in fact your patch is indeed required to make the new semantics work.
ok it's all clear now, thanks for clarifying. I've defined all the 'tmp' iterator variables uninitialized so applying your patch on top of that later will just give the nice compiler warning if they are used past the loop body.
What I disliked was always the maintainability of your patch - making the rules be something that isn't actually visible in the source code, and letting the old semantics still work as well as they ever did, and having to basically run some verification pass to find bad users.
Since this patch is not a complete list of cases that need fixing (30%) I haven't included the actual change of moving the iterator variable into the loop and thought that would be a second step coming after this is merged.
With these changes alone, yes you still rely on manual verification passes.
(I also disliked your original patch that mixed up the "CPU speculation type safety" with the actual non-speculative problems, but that was another issue).
Linus
- Jakob
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
On 1. Mar 2022, at 01:41, Linus Torvalds torvalds@linux-foundation.org wrote:
On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel jakobkoschel@gmail.com wrote:
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered using some automated checker tool, but I don't think that works from a long-term maintenance perspective.
So if we have the basic rule being "don't use the loop iterator after the loop has finished, because it can cause all kinds of subtle issues", then in _addition_ to fixing the existing code paths that have this issue, I really would want to (a) get a compiler warning for future cases and (b) make it not actually _work_ for future cases.
Because otherwise it will just happen again.
Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already have a patch for due to your automated tool. There were certainly more than a handful, but it didn't look _too_ bad to just make the rule be "don't use the iterator after the loop".
Of course, that's just based on that patch of yours. Maybe there are a ton of other cases that your patch didn't change, because they didn't match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing in total. I'll now finish all of them and then split them by submodules as Greg suggested and repost a patch set per submodule. Sounds good?
Sounds good to me!
If you need help carving these up and maintaining them over time as different subsystem maintainers accept/ignore them, just let me know. Doing large patchsets like this can be tough without a lot of experience.
thanks,
greg k-h
On 1. Mar 2022, at 18:36, Greg KH greg@kroah.com wrote:
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
On 1. Mar 2022, at 01:41, Linus Torvalds torvalds@linux-foundation.org wrote:
On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel jakobkoschel@gmail.com wrote:
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered using some automated checker tool, but I don't think that works from a long-term maintenance perspective.
So if we have the basic rule being "don't use the loop iterator after the loop has finished, because it can cause all kinds of subtle issues", then in _addition_ to fixing the existing code paths that have this issue, I really would want to (a) get a compiler warning for future cases and (b) make it not actually _work_ for future cases.
Because otherwise it will just happen again.
Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already have a patch for due to your automated tool. There were certainly more than a handful, but it didn't look _too_ bad to just make the rule be "don't use the iterator after the loop".
Of course, that's just based on that patch of yours. Maybe there are a ton of other cases that your patch didn't change, because they didn't match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing in total. I'll now finish all of them and then split them by submodules as Greg suggested and repost a patch set per submodule. Sounds good?
Sounds good to me!
If you need help carving these up and maintaining them over time as different subsystem maintainers accept/ignore them, just let me know. Doing large patchsets like this can be tough without a lot of experience.
Very much appreciated!
There will probably be some cases that do not match one of the pattern we already discussed and need separate attention.
I was planning to start with one subsystem and adjust the coming ones according to the feedback gather there instead of posting all of them in one go.
thanks,
greg k-h
- Jakob
On Tue, Mar 01, 2022 at 06:40:04PM +0100, Jakob Koschel wrote:
On 1. Mar 2022, at 18:36, Greg KH greg@kroah.com wrote:
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
On 1. Mar 2022, at 01:41, Linus Torvalds torvalds@linux-foundation.org wrote:
On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel jakobkoschel@gmail.com wrote:
The goal of this is to get compiler warnings right? This would indeed be great.
Yes, so I don't mind having a one-time patch that has been gathered using some automated checker tool, but I don't think that works from a long-term maintenance perspective.
So if we have the basic rule being "don't use the loop iterator after the loop has finished, because it can cause all kinds of subtle issues", then in _addition_ to fixing the existing code paths that have this issue, I really would want to (a) get a compiler warning for future cases and (b) make it not actually _work_ for future cases.
Because otherwise it will just happen again.
Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()).
So I have no problems with breaking cases that we basically already have a patch for due to your automated tool. There were certainly more than a handful, but it didn't look _too_ bad to just make the rule be "don't use the iterator after the loop".
Of course, that's just based on that patch of yours. Maybe there are a ton of other cases that your patch didn't change, because they didn't match your trigger case, so I may just be overly optimistic here.
Based on the coccinelle script there are ~480 cases that need fixing in total. I'll now finish all of them and then split them by submodules as Greg suggested and repost a patch set per submodule. Sounds good?
Sounds good to me!
If you need help carving these up and maintaining them over time as different subsystem maintainers accept/ignore them, just let me know. Doing large patchsets like this can be tough without a lot of experience.
Very much appreciated!
There will probably be some cases that do not match one of the pattern we already discussed and need separate attention.
I was planning to start with one subsystem and adjust the coming ones according to the feedback gather there instead of posting all of them in one go.
That seems wise. Feel free to use USB as a testing ground for this if you want to :)
thanks,
greg k-h
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote:
Based on the coccinelle script there are ~480 cases that need fixing in total. I'll now finish all of them and then split them by submodules as Greg suggested and repost a patch set per submodule. Sounds good?
To help with this splitting, see: https://github.com/kees/kernel-tools/blob/trunk/split-on-maintainer
It's not perfect, but it'll get you really close. For example, if you had a single big tree-wide patch applied to your tree:
$ rm 0*.patch $ git format-patch -1 HEAD $ mv 0*.patch treewide.patch $ split-on-maintainer treewide.patch $ ls 0*.patch
If you have a build log before the patch that spits out warnings, the --build-log argument can extract those warnings on a per-file basis, too (though this can be fragile).
On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds torvalds@linux-foundation.org wrote:
But basically to _me_, the important part is that the end result is maintainable longer-term.
I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member)
I have posted a patch series here to demonstrate this approach: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/
Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_inside, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_inside* one. At that time, we can just remove the implements of origin macros and rename the *_inside* macro back to the origin name just in one single patch.
And the "type(pos) pos" approach need teach developers to "not initialize the iterator variable, otherwise the use-after-loop will not be reported by compiler", which is unreasonable and impossible for all developers.
And it will mess up the following code logic and no warnning reported by compiler, even without initializing "ext" at the beginning: void foo(struct mem_extent *arg) { struct mem_extent *ext; // used both for iterator and normal ptr ... ext = arg; // this assignment can alse be done in another bar() func ... list_for_each_entry(ext, head, member) { if (found(ext)) break; } ... // use ext after the loop ret = ext; } If the loop hit the break, the last "ret" will be the found ext iterator. However, if the "type(pos) pos" approach applied, the last "ret" will be "arg" which is not the intention of the developers, because the "ext" is two different variables inside and outside the loop.
Thus, my idea is *better a finger off than always aching*, let's choose the "list_for_each_entry_inside(pos, type, head, member)" approach.
It turns out that just syntactically, it's really nice to give the type of the iterator from outside the way we do now. Yeah, it may be a bit odd, and maybe it's partly because I'm so used to the "list_for_each_list_entry()" syntax, but moving the type into the loop construct really made it nasty - either one very complex line, or having to split it over two lines which was even worse.
Maybe the place I looked at just happened to have a long typename, but it's basically always going to be a struct, so it's never a _simple_ type. And it just looked very odd adn unnatural to have the type as one of the "arguments" to that list_for_each_entry() macro.
we can pass a shorter type name to list_for_each_entry_inside, thus no need to split it over two lines. Actually it is not a big problem. + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_inside(pos, t, head, member) {
I put the type at the second argument not the first to avoid messing up the pattern match in some coccinelle scripts.
(b) gives us a nice warning for any normal use-after-loop case (unless you explicitly initialized it like that sgx_mmu_notifier_release() function did for no good reason
sometimes developers can be confused by the reported warnning: "used without having been initialized", and can not figure out immediately that "oh, now i am using another different variable but with the same name of the loop iterator variable", which has changed the programming habits of developers.
(c) also guarantees that even if you don't get a warning, non-converted (or newly written) bad code won't actually _work_
so you end up getting the new rules without any ambiguity or mistaken
It will lead to a wrong/NULL pointer dereference if the pointer is used anywhere else, depend on which value is used to initialized with.
Best regard, -- Xiaomeng Tong
From: Xiaomeng Tong
Sent: 02 March 2022 09:31
On Mon, 28 Feb 2022 16:41:04 -0800, Linus Torvalds torvalds@linux-foundation.org wrote:
But basically to _me_, the important part is that the end result is maintainable longer-term.
I couldn't agree more. And because of that, I stick with the following approach because it's maintainable longer-term than "type(pos) pos" one: Implements a new macro for each list_for_each_entry* with _inside suffix. #define list_for_each_entry_inside(pos, type, head, member)
I think that it would be better to make any alternate loop macro just set the variable to NULL on the loop exit. That is easier to code for and the compiler might be persuaded to not redo the test.
It also doesn't need an extra variable defined in the for() statement so can be back-ported to older kernels without required declaration in the middle of blocks.
OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, 2 Mar 2022 14:04:06 +0000, David Laight David.Laight@ACULAB.COM wrote:
I think that it would be better to make any alternate loop macro just set the variable to NULL on the loop exit. That is easier to code for and the compiler might be persuaded to not redo the test.
No, that would lead to a NULL dereference.
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
IOW, you would dereference a (NULL + offset_of_member) address here.
Please remind me if i missed something, thanks.
OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel.
The "list_for_each_entry_inside(pos, type, head, member)" way makes the iterator invisiable outside the loop, and would be catched by compiler if use-after-loop things happened.
Can you share your "alternative definitions" details? thanks!
-- Xiaomeng Tong
From: Xiaomeng Tong
Sent: 03 March 2022 02:27
On Wed, 2 Mar 2022 14:04:06 +0000, David Laight David.Laight@ACULAB.COM wrote:
I think that it would be better to make any alternate loop macro just set the variable to NULL on the loop exit. That is easier to code for and the compiler might be persuaded to not redo the test.
No, that would lead to a NULL dereference.
Why, it would make it b ethe same as the 'easy to use': for (item = head; item; item = item->next) { ... if (...) break; ... } if (!item) return;
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never be calculated at all. This is what is fundamentally broken about the current definition.
IOW, you would dereference a (NULL + offset_of_member) address here.
Where?
Please remind me if i missed something, thanks.
Can you share your "alternative definitions" details? thanks!
The loop should probably use as extra variable that points to the 'list node' in the next structure. Something like: for (xxx *iter = head->next; iter == &head ? ((item = NULL),0) : ((item = list_item(iter),1)); iter = item->member->next) { ... With a bit of casting you can use 'item' to hold 'iter'.
OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel.
The "list_for_each_entry_inside(pos, type, head, member)" way makes the iterator invisiable outside the loop, and would be catched by compiler if use-after-loop things happened.
It is also a compete PITA for anything doing a search.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never be calculated at all. This is what is fundamentally broken about the current definition.
Yes, the rule is "the HEAD's container_of pointer should never be calculated at all outside the loop", but how do you make sure everyone follows this rule? Everyone makes mistakes, but we can eliminate them all from the beginning with the help of compiler which can catch such use-after-loop things.
IOW, you would dereference a (NULL + offset_of_member) address here.
Where?
In the case where a developer do not follows the above rule, and mistakely access a non-list-head member of the HEAD's container_of pointer outside the loop. For example: struct req{ int a; struct list_head h; } struct req *r; list_for_each_entry(r, HEAD, h) { if (r->a == 0x10) break; } // the developer made a mistake: he didn't take this situation into // account where all entries in the list are *r->a != 0x10*, and now // the r is the HEAD's container_of pointer. r->a = 0x20; Thus the "r->a = 0x20" would dereference a (NULL + offset_of_member) address here.
Please remind me if i missed something, thanks.
Can you share your "alternative definitions" details? thanks!
The loop should probably use as extra variable that points to the 'list node' in the next structure. Something like: for (xxx *iter = head->next; iter == &head ? ((item = NULL),0) : ((item = list_item(iter),1)); iter = item->member->next) { ... With a bit of casting you can use 'item' to hold 'iter'.
you still can not make sure everyone follows this rule: "do not use iterator outside the loop" without the help of compiler, because item is declared outside the loop.
BTW, to avoid ambiguity，the "alternative definitions" here i asked is something from you in this context: "OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel."
OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel.
The "list_for_each_entry_inside(pos, type, head, member)" way makes the iterator invisiable outside the loop, and would be catched by compiler if use-after-loop things happened.
It is also a compete PITA for anything doing a search.
You mean it would be a burden on search? can you show me some examples?
Or you mean it is too long the list_for_each_entry_inside* string to live in one single line, and should spilt into two line? If it is the case, there are 2 way to mitigate it. 1. pass a shorter t as struct type to the macro 2. after all list_for_each_entry macros be replaced with list_for_each_entry_inside, remove the list_for_each_entry implementations and rename all list_for_each_entry_inside use back to the origin list_for_each_entry in a single patch.
For me, it is acceptable and not a such big problem.
-- Xiaomeng Tong
From: Xiaomeng Tong
Sent: 03 March 2022 07:27
On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never be calculated at all. This is what is fundamentally broken about the current definition.
Yes, the rule is "the HEAD's container_of pointer should never be calculated at all outside the loop", but how do you make sure everyone follows this rule? Everyone makes mistakes, but we can eliminate them all from the beginning with the help of compiler which can catch such use-after-loop things.
IOW, you would dereference a (NULL + offset_of_member) address here.
Where?
In the case where a developer do not follows the above rule, and mistakely access a non-list-head member of the HEAD's container_of pointer outside the loop. For example: struct req{ int a; struct list_head h; } struct req *r; list_for_each_entry(r, HEAD, h) { if (r->a == 0x10) break; } // the developer made a mistake: he didn't take this situation into // account where all entries in the list are *r->a != 0x10*, and now // the r is the HEAD's container_of pointer. r->a = 0x20; Thus the "r->a = 0x20" would dereference a (NULL + offset_of_member) address here.
That is just a bug. No different to failing to check anything else might 'return' a NULL pointer. Because it is a NULL dereference you find out pretty quickly. The existing loop leaves you with a valid pointer to something that isn't a list item.
Please remind me if i missed something, thanks.
Can you share your "alternative definitions" details? thanks!
The loop should probably use as extra variable that points to the 'list node' in the next structure. Something like: for (xxx *iter = head->next; iter == &head ? ((item = NULL),0) : ((item = list_item(iter),1)); iter = item->member->next) { ... With a bit of casting you can use 'item' to hold 'iter'.
you still can not make sure everyone follows this rule: "do not use iterator outside the loop" without the help of compiler, because item is declared outside the loop.
That one has 'iter' defined in the loop.
BTW, to avoid ambiguity，the "alternative definitions" here i asked is something from you in this context: "OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel."
I was thinking of something like: if ((pos = list_first)), 1) pos = NULL else so that unchecked dereferences after the loop will be detectable as NULL pointer offsets - but that in itself isn't enough to avoid other warnings.
The "list_for_each_entry_inside(pos, type, head, member)" way makes the iterator invisiable outside the loop, and would be catched by compiler if use-after-loop things happened.
It is also a compete PITA for anything doing a search.
You mean it would be a burden on search? can you show me some examples?
The whole business of having to save the pointer to the located item before breaking the loop, remembering to have set it to NULL earlier etc.
It is so much better if you can just do: if (found) break;
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Xiaomeng Tong
Sent: 03 March 2022 07:27
On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never be calculated at all. This is what is fundamentally broken about the current definition.
Yes, the rule is "the HEAD's container_of pointer should never be calculated at all outside the loop", but how do you make sure everyone follows this rule? Everyone makes mistakes, but we can eliminate them all from the beginning with the help of compiler which can catch such use-after-loop things.
IOW, you would dereference a (NULL + offset_of_member) address here.
Where?
In the case where a developer do not follows the above rule, and mistakely access a non-list-head member of the HEAD's container_of pointer outside the loop. For example: struct req{ int a; struct list_head h; } struct req *r; list_for_each_entry(r, HEAD, h) { if (r->a == 0x10) break; } // the developer made a mistake: he didn't take this situation into // account where all entries in the list are *r->a != 0x10*, and now // the r is the HEAD's container_of pointer. r->a = 0x20; Thus the "r->a = 0x20" would dereference a (NULL + offset_of_member) address here.
That is just a bug. No different to failing to check anything else might 'return' a NULL pointer.
Yes, but it‘s a mistake everyone has made and will make, we should avoid this at the beginning with the help of compiler.
Because it is a NULL dereference you find out pretty quickly.
AFAIK，NULL dereference is a undefined bahavior, can compiler quickly catch it? Or it can only be applied to some simple/restricted cases.
The existing loop leaves you with a valid pointer to something that isn't a list item.
Please remind me if i missed something, thanks.
Can you share your "alternative definitions" details? thanks!
The loop should probably use as extra variable that points to the 'list node' in the next structure. Something like: for (xxx *iter = head->next; iter == &head ? ((item = NULL),0) : ((item = list_item(iter),1)); iter = item->member->next) { ... With a bit of casting you can use 'item' to hold 'iter'.
you still can not make sure everyone follows this rule: "do not use iterator outside the loop" without the help of compiler, because item is declared outside the loop.
That one has 'iter' defined in the loop.
Oh, sorry, I misunderstood. Then this is the same way with my way of list_for_each_entry_inside(pos, type, head, member), which declare the iterator inside the loop. You go further and make things like "&pos->member == (head)" goes away to avoid calculate the HEAD's container_of pointer, although the calculation is harmless.
BTW, to avoid ambiguity，the "alternative definitions" here i asked is something from you in this context: "OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel."
I was thinking of something like: if ((pos = list_first)), 1) pos = NULL else so that unchecked dereferences after the loop will be detectable as NULL pointer offsets - but that in itself isn't enough to avoid other warnings.
Do you mean put this right after the loop (I changed somthing that i do not understand, please correct me if i am worng, thanks): if (pos == list_first) pos = NULL; else and compiler can detect the following NULL derefernce? But if the NULL derefernce is just right after the loop originally, it will be masked by the *else* keyword。
The "list_for_each_entry_inside(pos, type, head, member)" way makes the iterator invisiable outside the loop, and would be catched by compiler if use-after-loop things happened.
It is also a compete PITA for anything doing a search.
You mean it would be a burden on search? can you show me some examples?
The whole business of having to save the pointer to the located item before breaking the loop, remembering to have set it to NULL earlier etc.
Ok, i see. And then you need pass a "item" to the list_for_each_entry macro as a new argument.
It is so much better if you can just do: if (found) break;
David
this confused me. this way is better or the "save the pointer to the located item before breaking the loop" one?
-- Xiaomeng Tong
On Thu, Mar 03, 2022 at 03:26:57PM +0800, Xiaomeng Tong wrote:
On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never be calculated at all. This is what is fundamentally broken about the current definition.
Yes, the rule is "the HEAD's container_of pointer should never be calculated at all outside the loop", but how do you make sure everyone follows this rule?
Your formulation of the rule is correct: never run container_of() on HEAD pointer.
However the rule that is introduced by list_for_each_entry_inside() is *not* this rule. The rule it introduces is: never access the iterator variable outside the loop.
Making the iterator NULL on loop exit does follow the rule you proposed but using a different technique: do not allow HEAD to be stored in the iterator variable after loop exit. This also makes it impossible to run container_of() on the HEAD pointer.
Everyone makes mistakes, but we can eliminate them all from the beginning with the help of compiler which can catch such use-after-loop things.
Indeed but if we introduce new interfaces then we don't have to worry about existing usages and silent regressions. Code will have been written knowing the loop can exit with the iterator set to NULL.
Sure it is still possible for programmers to make mistakes and dereference the NULL pointer but C programmers are well training w.r.t. NULL pointer checking so such mistakes are much less likely than with the current list_for_each_entry() macro. This risk must be offset against the way a NULLify approach can lead to more elegant code when we are doing a list search.
Daniel.
On Thu, 3 Mar 2022 12:18:24 +0000, Daniel Thompson wrote:
On Thu, Mar 03, 2022 at 03:26:57PM +0800, Xiaomeng Tong wrote:
On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never be calculated at all. This is what is fundamentally broken about the current definition.
Yes, the rule is "the HEAD's container_of pointer should never be calculated at all outside the loop", but how do you make sure everyone follows this rule?
Your formulation of the rule is correct: never run container_of() on HEAD pointer.
Actually, it is not my rule. My rule is that never access other members of the struct except for the list_head member after the loop, because this is a invalid member after loop exit, but valid for the list_head member which just is HEAD and the lately caculation (&pos->head) seems harmless.
I have considered the case that the HEAD's container "pos" is layouted across the max and the min address boundary, which means the address of HEAD is likely 0x60, and the address of pos is likely 0xffffffe0. It seems ok to caculate pos with: ((type *)(__mptr - offsetof(type, member))); and it seems ok to caculate head outside the loop with: if (&pos->head == &HEAD) return NULL;
The only case I can think of with the rule "never run container_of() on HEAD" must be followed is when the first argument (which is &HEAD) passing to container_of() is NULL + some offset, it may lead to the resulting "pos->member" access being a NULL dereference. But maybe the caller can take the responsibility to check if it is NULL, not container_of() itself.
Please remind me if i missed somthing, thanks.
However the rule that is introduced by list_for_each_entry_inside() is *not* this rule. The rule it introduces is: never access the iterator variable outside the loop.
Sorry for the confusion, indeed, that is two *different* rule.
Making the iterator NULL on loop exit does follow the rule you proposed but using a different technique: do not allow HEAD to be stored in the iterator variable after loop exit. This also makes it impossible to run container_of() on the HEAD pointer.
It does not. My rule is: never access the iterator variable outside the loop. The "Making the iterator NULL on loop exit" way still leak the pos with NULL outside the loop, may lead to a NULL deference.
Everyone makes mistakes, but we can eliminate them all from the beginning with the help of compiler which can catch such use-after-loop things.
Indeed but if we introduce new interfaces then we don't have to worry about existing usages and silent regressions. Code will have been written knowing the loop can exit with the iterator set to NULL.
Yes, it is more simple and compatible with existing interfaces. Howerver, you should make every developers to remember that "pos will be set NULL on loop exit", which is unreasonable and impossible for *every* single person. Otherwise the mis-use-after-loop will lead to a NULL dereference. But we can kill this problem by declaring iterator inside the loop and the complier will catch it if somebody mis-use-after-loop.
Sure it is still possible for programmers to make mistakes and dereference the NULL pointer but C programmers are well training w.r.t. NULL pointer checking so such mistakes are much less likely than with the current list_for_each_entry() macro. This risk must be offset against the way a NULLify approach can lead to more elegant code when we are doing a list search.
Yes, the NULLify approach is better than the current list_for_each_entry() macro, but i stick with that the list_for_each_entry_inside() way is best and perfect _technically_.
Thus, my idea is *better a finger off than always aching*, let's settle this damn problem once and for all, with list_for_each_entry_inside().
-- Xiaomeng Tong
On 3. Mar 2022, at 05:58, David Laight David.Laight@ACULAB.COM wrote:
From: Xiaomeng Tong
Sent: 03 March 2022 02:27
On Wed, 2 Mar 2022 14:04:06 +0000, David Laight David.Laight@ACULAB.COM wrote:
I think that it would be better to make any alternate loop macro just set the variable to NULL on the loop exit. That is easier to code for and the compiler might be persuaded to not redo the test.
No, that would lead to a NULL dereference.
Why, it would make it b ethe same as the 'easy to use': for (item = head; item; item = item->next) { ... if (...) break; ... } if (!item) return;
The problem is the mis-use of iterator outside the loop on exit, and the iterator will be the HEAD's container_of pointer which pointers to a type-confused struct. Sidenote: The *mis-use* here refers to mistakely access to other members of the struct, instead of the list_head member which acutally is the valid HEAD.
The problem is that the HEAD's container_of pointer should never be calculated at all. This is what is fundamentally broken about the current definition.
IOW, you would dereference a (NULL + offset_of_member) address here.
Where?
Please remind me if i missed something, thanks.
Can you share your "alternative definitions" details? thanks!
The loop should probably use as extra variable that points to the 'list node' in the next structure. Something like: for (xxx *iter = head->next; iter == &head ? ((item = NULL),0) : ((item = list_item(iter),1)); iter = item->member->next) { ... With a bit of casting you can use 'item' to hold 'iter'.
I think this would make sense, it would mean you only assign the containing element on valid elements.
I was thinking something along the lines of:
#define list_for_each_entry(pos, head, member) \ for (struct list_head *list = head->next, typeof(pos) pos; \ list == head ? 0 : (( pos = list_entry(pos, list, member), 1)); \ list = list->next)
Although the initialization block of the for loop is not valid C, I'm not sure there is any way to declare two variables of a different type in the initialization part of the loop.
I believe all this does is get rid of the &pos->member == (head) check to terminate the list. It alone will not fix any of the other issues that using the iterator variable after the loop currently has.
AFAIK Adrián Moreno is working on doing something along those lines for the list iterator in openvswitch (that was similar to the kernel one before) [1].
I *think* they don't declare 'pos' within the loop which we *do want* to avoid any uses of it after the loop. (If pos is not declared in the initialization block, shadowing the *outer* pos, it would just point to the last element of the list or stay uninitialized if the list is empty).
[1] https://www.mail-archive.com/ovs-dev@openvswitch.org/msg63497.html
OTOH there may be alternative definitions that can be used to get the compiler (or other compiler-like tools) to detect broken code. Even if the definition can't possibly generate a working kerrnel.
The "list_for_each_entry_inside(pos, type, head, member)" way makes the iterator invisiable outside the loop, and would be catched by compiler if use-after-loop things happened.
It is also a compete PITA for anything doing a search.
David
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
- Jakob
I think this would make sense, it would mean you only assign the containing element on valid elements.
I was thinking something along the lines of:
#define list_for_each_entry(pos, head, member) \ for (struct list_head *list = head->next, typeof(pos) pos; \ list == head ? 0 : (( pos = list_entry(pos, list, member), 1)); \ list = list->next)
Although the initialization block of the for loop is not valid C, I'm not sure there is any way to declare two variables of a different type in the initialization part of the loop.
It can be done using a *nested loop*, like this:
#define list_for_each_entry(pos, head, member) \ for (struct list_head *list = head->next, cond = (struct list_head *)-1; cond == (struct list_head *)-1; cond = NULL) \ for (typeof(pos) pos; \ list == head ? 0 : (( pos = list_entry(pos, list, member), 1)); \ list = list->next)
I believe all this does is get rid of the &pos->member == (head) check to terminate the list.
Indeed, although the original way is harmless.
It alone will not fix any of the other issues that using the iterator variable after the loop currently has.
Yes, but I stick with the list_for_each_entry_inside(pos, type, head, member) way to make the iterator invisiable outside the loop (before and after the loop). It is maintainable longer-term than "type(pos) pos" one and perfect. see my explain: https://lore.kernel.org/lkml/20220302093106.8402-1-xiam0nd.tong@gmail.com/ and list_for_each_entry_inside(pos, type, head, member) patch here: https://lore.kernel.org/lkml/20220301075839.4156-3-xiam0nd.tong@gmail.com/
-- Xiaomeng Tong
correct for typo:
-for (struct list_head *list = head->next, cond = (struct list_head *)-1; cond == (struct list_head *)-1; cond = NULL) \ +for (struct list_head *list = head->next, *cond = (struct list_head *)-1; cond == (struct list_head *)-1; cond = NULL) \
-- Xiaomeng Tong
participants (11)
-
Daniel Thompson -
David Laight -
Greg KH -
Jakob Koschel -
Jakub Kicinski -
Johannes Berg -
Kees Cook -
Linus Torvalds -
Matthew Wilcox -
Segher Boessenkool -
Xiaomeng Tong