| Message ID | 20220228110822.491923-1-jakobkoschel@gmail.com |
|---|---|
| Headers | show |
| Series | Remove usage of list iterator past the loop body | expand |
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
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:10 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > 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. 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:10:24PM -0800, Linus Torvalds wrote: > 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. 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) 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. > > 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, 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. James
Am 28.02.22 um 21:42 schrieb James Bottomley: > 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: >>> [SNIP] >>> 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. That will work and is also what people already do. The key problem is that we let people do the same thing over and over again with slightly different implementations. Out in the wild I've seen at least using a separate variable, using a bool to indicate that something was found and just assuming that the list has an entry. The last case is bogus and basically what can break badly. If we would have an unified macro which search for an entry combined with automated reporting on patches to use that macro I think the potential to introduce such issues will already go down massively without auditing tons of existing code. Regards, Christian. > > James > >
On Mon, Feb 28, 2022 at 3:45 PM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > ... > > 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); Borrowing from c++, perhaps an explicit end should be used: if (list_entry_not_end(entry)) do_somethin_with(entry) It is modelled after c++ and the 'while(begin != end) {}' pattern. Jeff
> On 28. Feb 2022, at 21:56, Christian König <christian.koenig@amd.com> wrote: > > > > Am 28.02.22 um 21:42 schrieb James Bottomley: >> 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: >>>> [SNIP] >>>> 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); There are other cases where the list iterator variable is used after the loop Some examples: - list_for_each_entry_continue() and list_for_each_entry_from(). - (although very rare) the head is actually of the correct struct type. (ppc440spe_get_group_entry(): drivers/dma/ppc4xx/adma.c:1436) - to use pos->list for example for list_add_tail(): (add_static_vm_early(): arch/arm/mm/ioremap.c:107) If the scope of the list iterator is limited those still need fixing in a different way. >> >> 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. > > That will work and is also what people already do. > > The key problem is that we let people do the same thing over and over again with slightly different implementations. > > Out in the wild I've seen at least using a separate variable, using a bool to indicate that something was found and just assuming that the list has an entry. > > The last case is bogus and basically what can break badly. > > If we would have an unified macro which search for an entry combined with automated reporting on patches to use that macro I think the potential to introduce such issues will already go down massively without auditing tons of existing code. Having a unified way to do the same thing would indeed be great. > > Regards, > Christian. > >> >> James >> >> > - Jakob
On Mon, 2022-02-28 at 23:59 +0200, Mike Rapoport wrote: > > 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: [...] > > > > 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. Well, yes, but my objection is more to the size of churn than the desire to do loop local. I'm not even sure loop local is possible, because it's always annoyed me that for (int i = 0; ... in C++ defines i in the outer scope not the loop scope, which is why I never use it. However, if the desire is really to poison the loop variable then we can do #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 = NULL) == NULL; \ pos = list_next_entry(pos, member)) Which would at least set pos to NULL when the loop completes. > 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. I get the wider goal, but we have to patch the problem cases now and a simple one-liner is better than a larger patch that may or may not work if we ever achieve the local definition or value poisoning idea. I'm also fairly certain coccinelle can come up with a use without checking for loop completion semantic patch which we can add to 0day. James
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:38 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > In C its scope is the rest of the declaration and the entire loop, not > anything after it. This was the same in C++98 already, btw (but in > pre-standard versions of C++ things were like you remember, yes, and it > was painful). Yeah, the original C++ model was just unadulterated garbage, with no excuse for it, and the scope was not the loop, but the block the loop existed in. That would never have been acceptable for the kernel - it's basically just an even uglier version of "put variable declarations in the middle of code" (and we use "-Wdeclaration-after-statement" to disallow that for kernel code, although apparently some of our user space tooling code doesn't enforce or follow that rule). The actual C99 version is the sane one which actually makes it easier and clearer to have loop iterators that are clearly just in loop scope. That's a good idea in general, and I have wanted to start using that in the kernel even aside from some of the loop construct macros. Because putting variables in natural minimal scope is a GoodThing(tm). Of course, we shouldn't go crazy with it. Even after we do that -std=gnu11 thing, we'll have backports to worry about. And it's not clear that we necessarily want to backport that gnu11 thing - since people who run old stable kernels also may be still using those really old compilers... 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
From: Linus Torvalds > Sent: 28 February 2022 19:56 > 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 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. I think you actually want: !list_entry_is_head(__iter, head, member) ? (((pos)=__iter),1) : (((pos) = NULL),0); Which can be done in the original by: !list_entry_is_head(pos, head, member) ? 1 : (((pos) = NULL), 0); Although it would be safer to have (without looking up the actual name): for (item *__item = head; \ __item ? (((pos) = list_item(__item, member)), 1) : (((pos) = NULL), 0); __item = (pos)->member) The local does need 'fixing' to avoid shadowing for nested loops. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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) 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. > > 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 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) 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 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...
Am 28.02.22 um 22:13 schrieb James Bottomley: > On Mon, 2022-02-28 at 21:56 +0100, Christian König wrote: >> Am 28.02.22 um 21:42 schrieb James Bottomley: >>> 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: >>>>> [SNIP] >>>>> 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. >> That will work and is also what people already do. >> >> The key problem is that we let people do the same thing over and >> over again with slightly different implementations. >> >> Out in the wild I've seen at least using a separate variable, using >> a bool to indicate that something was found and just assuming that >> the list has an entry. >> >> The last case is bogus and basically what can break badly. > Yes, I understand that. I'm saying we should replace that bogus checks > of entry->something against some_value loop termination condition with > the list_entry_is_head() macro. That should be a one line and fairly > mechanical change rather than the explosion of code changes we seem to > have in the patch series. Yes, exactly that's my thinking as well. Christian. > > James > >
> 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) 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. 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 Mon, Feb 28, 2022 at 01:06:57PM +0100, Jakob Koschel wrote: > > > > On 28. Feb 2022, at 12:20, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: > >> If the list does not contain the expected element, the value of > >> list_for_each_entry() iterator will not point to a valid structure. > >> To avoid type confusion in such case, the list iterator > >> scope will be limited to list_for_each_entry() loop. > >> > >> In preparation to limiting scope of a list iterator to the list traversal > >> loop, use a dedicated pointer to point to the found element. > >> Determining if an element was found is then simply checking if > >> the pointer is != NULL. > >> > >> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> > >> --- > >> arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- > >> drivers/scsi/scsi_transport_sas.c | 17 ++++++++----- > >> drivers/thermal/thermal_core.c | 38 ++++++++++++++++++---------- > >> drivers/usb/gadget/configfs.c | 22 ++++++++++------ > >> drivers/usb/gadget/udc/max3420_udc.c | 11 +++++--- > >> drivers/usb/gadget/udc/tegra-xudc.c | 11 +++++--- > >> drivers/usb/mtu3/mtu3_gadget.c | 11 +++++--- > >> drivers/usb/musb/musb_gadget.c | 11 +++++--- > >> drivers/vfio/mdev/mdev_core.c | 11 +++++--- > >> 9 files changed, 88 insertions(+), 50 deletions(-) > > > > The drivers/usb/ portion of this patch should be in patch 1/X, right? > > I kept them separate since it's a slightly different case. > Using 'ptr' instead of '&ptr->other_member'. Regardless, I'll split > this commit up as you mentioned. > > > > > Also, you will have to split these up per-subsystem so that the > > different subsystem maintainers can take these in their trees. > > Thanks for the feedback. > To clarify I understand you correctly: > I should do one patch set per-subsystem with every instance/(file?) > fixed as a separate commit? Yes, each file needs a different commit as they are usually all written or maintained by different people. As I said in my other response, if you need any help with this, just let me know, I'm glad to help. 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 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 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 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 Mon, Feb 28, 2022 at 2:29 PM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > However, if the desire is really to poison the loop variable then we > can do > > #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 = NULL) == NULL; \ > pos = list_next_entry(pos, member)) > > Which would at least set pos to NULL when the loop completes. That would actually have been excellent if we had done that originally. It would not only avoid the stale and incorrectly typed head entry left-over turd, it would also have made it very easy to test for "did I find an entry in the loop". But I don't much like it in the situation we are now. Why? Mainly because it basically changes the semantics of the loop _without_ any warnings about it. And we don't actually get the advantage of the nicer semantics, because we can't actually make code do list_for_each_entry(entry, ....) { .. } if (!entry) return -ESRCH; .. use the entry we found .. because that would be a disaster for back-porting, plus it would be a flag-day issue (ie we'd have to change the semantics of the loop at the same time we change every single user). So instead of that simple "if (!entry)", we'd effectively have to continue to use something that still works with the old world order (ie that "if (list_entry_is_head())" model). So we couldn't really take _advantage_ of the nicer semantics, and we'd not even get a warning if somebody does it wrong - the code would just silently do the wrong thing. IOW: I don't think you are wrong about that patch: it would solve the problem that Jakob wants to solve, and it would have absolutely been much better if we had done this from the beginning. But I think that in our current situation, it's actually a really fragile solution to the "don't do that then" problem we have. Linus
On Tue, Mar 1, 2022 at 11:06 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So instead of that simple "if (!entry)", we'd effectively have to > continue to use something that still works with the old world order > (ie that "if (list_entry_is_head())" model). Just to prove my point about how this is painful, that doesn't work at all. If the loop iterator at the end is NULL (good, in theory), we can't use "list_entry_is_head()" to check whether we ended. We'd have to use a new thing entirely, to handle the "list_for_each_entry() has the old/new semantics" cases. That's largely why I was pushing for the "let's make it impossible to use the loop iterator at all outside the loop". It avoids the confusing case, and the patches to move to that stricter semantic can be merged independently (and before) doing the actual semantic change. I'm not saying my suggested approach is wonderful either. Honestly, it's painful that we have so nasty semantics for the end-of-loop case for list_for_each_entry(). The minimal patch would clearly be to keep those broken semantics, and just force everybody to use the list_entry_is_head() case. That's the "we know we messed up, we are too lazy to fix it, we'll just work around it and people need to be careful" approach. And laziness is a virtue. But bad semantics are bad semantics. So it's a question of balancing those two issues. Linus
So looking at this patch, I really reacted to the fact that quite often the "use outside the loop" case is all kinds of just plain unnecessary, but _used_ to be a convenience feature. I'll just quote the first chunk in it's entirely as an example - not because I think this chunk is particularly important, but because it's a good example: On Mon, Feb 28, 2022 at 3:09 AM Jakob Koschel <jakobkoschel@gmail.com> wrote: > > diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c > index 6794e2db1ad5..fc47c107059b 100644 > --- a/arch/arm/mach-mmp/sram.c > +++ b/arch/arm/mach-mmp/sram.c > @@ -39,19 +39,22 @@ static LIST_HEAD(sram_bank_list); > struct gen_pool *sram_get_gpool(char *pool_name) > { > struct sram_bank_info *info = NULL; > + struct sram_bank_info *tmp; > > if (!pool_name) > return NULL; > > mutex_lock(&sram_lock); > > - list_for_each_entry(info, &sram_bank_list, node) > - if (!strcmp(pool_name, info->pool_name)) > + list_for_each_entry(tmp, &sram_bank_list, node) > + if (!strcmp(pool_name, tmp->pool_name)) { > + info = tmp; > break; > + } > > mutex_unlock(&sram_lock); > > - if (&info->node == &sram_bank_list) > + if (!info) > return NULL; > > return info->gpool; I realize this was probably at least auto-generated with coccinelle, but maybe that script could be taught to do avoid the "use after loop" by simply moving the code _into_ the loop. IOW, this all would be cleaner and clear written as if (!pool_name) return NULL; mutex_lock(&sram_lock); list_for_each_entry(info, &sram_bank_list, node) { if (!strcmp(pool_name, info->pool_name)) { mutex_unlock(&sram_lock); return info; } } mutex_unlock(&sram_lock); return NULL; Ta-daa - no use outside the loop, no need for new variables, just a simple "just do it inside the loop". Yes, we end up having that lock thing twice, but it looks worth it from a "make the code obvious" standpoint. Would it be even cleaner if the locking was done in the caller, and the loop was some simple helper function? It probably would. But that would require a bit more smarts than probably a simple coccinelle script would do. Linus
From: Linus Torvalds > Sent: 01 March 2022 19:07 > On Mon, Feb 28, 2022 at 2:29 PM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > However, if the desire is really to poison the loop variable then we > > can do > > > > #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 = NULL) == NULL; \ > > pos = list_next_entry(pos, member)) > > > > Which would at least set pos to NULL when the loop completes. > > That would actually have been excellent if we had done that > originally. It would not only avoid the stale and incorrectly typed > head entry left-over turd, it would also have made it very easy to > test for "did I find an entry in the loop". > > But I don't much like it in the situation we are now. > > Why? Mainly because it basically changes the semantics of the loop > _without_ any warnings about it. And we don't actually get the > advantage of the nicer semantics, because we can't actually make code > do > > list_for_each_entry(entry, ....) { > .. > } > if (!entry) > return -ESRCH; > .. use the entry we found .. > > because that would be a disaster for back-porting, plus it would be a > flag-day issue (ie we'd have to change the semantics of the loop at > the same time we change every single user). > > So instead of that simple "if (!entry)", we'd effectively have to > continue to use something that still works with the old world order > (ie that "if (list_entry_is_head())" model). > > So we couldn't really take _advantage_ of the nicer semantics, and > we'd not even get a warning if somebody does it wrong - the code would > just silently do the wrong thing. > > IOW: I don't think you are wrong about that patch: it would solve the > problem that Jakob wants to solve, and it would have absolutely been > much better if we had done this from the beginning. But I think that > in our current situation, it's actually a really fragile solution to > the "don't do that then" problem we have. Can it be resolved by making: #define list_entry_is_head(pos, head, member) ((pos) == NULL) and double-checking that it isn't used anywhere else (except in the list macros themselves). The odd ones I just found are fs/locks.c mm/page_reporting.c security/apparmor/apparmorfs.c (3 times) net/xfrm/xfrm_ipcomp.c#L244 is buggy. (There is a WARN_ON() then it just carries on regardless!) There are only about 25 uses of list_entry_is_head(). There are a lot more places where these lists seem to be scanned by hand. I bet a few of those aren't actually right either. (Oh at 3am this morning I thought it was a different list type that could have much the same problem!) Another plausible solution is a variant of list_foreach_entry() that does set the 'entry' to NULL at the end. Then code can be moved over in stages. I'd reorder the arguments as well as changing the name! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Mar 1, 2022 at 2:58 PM David Laight <David.Laight@aculab.com> wrote: > > Can it be resolved by making: > #define list_entry_is_head(pos, head, member) ((pos) == NULL) > and double-checking that it isn't used anywhere else (except in > the list macros themselves). Well, yes, except for the fact that then the name is entirely misleading... And somebody possibly uses it together with list_first_entry() etc, so it really is completely broken to mix that change with the list traversal change. Linus Linus
From: Linus Torvalds > Sent: 01 March 2022 23:03 > > On Tue, Mar 1, 2022 at 2:58 PM David Laight <David.Laight@aculab.com> wrote: > > > > Can it be resolved by making: > > #define list_entry_is_head(pos, head, member) ((pos) == NULL) > > and double-checking that it isn't used anywhere else (except in > > the list macros themselves). > > Well, yes, except for the fact that then the name is entirely misleading... > > And somebody possibly uses it together with list_first_entry() etc, so > it really is completely broken to mix that change with the list > traversal change. Probably true :-( Actually adding list_entry_not_found() as a synonym for list_entry_is_head() and changing the 25ish places that use it after a loop might work. Once that is done the loop can be changed at the same time as list_entry_not_found(). That won't affect the in-tree callers. (and my out of tree modules don't use those lists - so I don't care about that!) Having said that there are so few users of list_entry_is_head() it is reasonable to generate two new names. One for use after list_for_each_entry() and one for list_next_entry(). Then the change all the call sites. After that list_entry_is_head() can be deleted - breaking out of tree compiles. Finally list_for_each_entry() can be rewritten to set NULL at the end of the list. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Mar 1, 2022 at 3:19 PM David Laight <David.Laight@aculab.com> wrote: > > Having said that there are so few users of list_entry_is_head() > it is reasonable to generate two new names. Well, the problem is that the users of list_entry_is_head() may be few - but there are a number of _other_ ways to check "was that the HEAD pointer", and not all of them are necessarily correct. IOW, different places do different random tests for "did we walk the whole loop without breaking out". And many of them happen to work. In fact, in practice, pretty much *all* of them happen to work, and you have to have the right struct layout and really really bad luck to hit a case of "type confusion ended up causing the test to not work". And *THAT* is the problem here. It's not the "there are 25ish places that current use list_entry_is_head()". It's the "there are ~480 places that use the type-confused HEAD entry that has been cast to the wrong type". And THAT is why I think we'd be better off with that bigger change that simply means that you can't use the iterator variable at all outside the loop, and try to make it something where the compiler can help catch mis-uses. Now, making the list_for_each_entry() thing force the iterator to NULL at the end of the loop does fix the problem. The issue I have with it is really just that you end up getting no warning at all from the compiler if you mix old-style and new-style semantics. Now, you *will* get an oops (if using a new-style iterator with an old-style check), but many of these things will be in odd driver code and may happen only for error cases. And if you use a new-style check with an old-style iterator (ie some backport problem), you will probably end up getting random memory corruption, because you'll decide "it's not a HEAD entry", and then you'll actually *use* the HEAD that has the wrong type cast associated with it. See what my worry is? With the "don't use iterator outside the loop" approach, the exact same code works in both the old world order and the new world order, and you don't have the semantic confusion. And *if* you try to use the iterator outside the loop, you'll _mostly_ (*) get a compiler warning about it not being initialized. Linus (*) Unless somebody initializes the iterator pointer pointlessly. Which clearly does happen. Thus the "mostly". It's not perfect, and that's most definitely not nice - but it should at least hopefully make it that much harder to mess up.
On 02/03/2022 00.55, Linus Torvalds wrote: > On Tue, Mar 1, 2022 at 3:19 PM David Laight <David.Laight@aculab.com> wrote: >> > With the "don't use iterator outside the loop" approach, the exact > same code works in both the old world order and the new world order, > and you don't have the semantic confusion. And *if* you try to use the > iterator outside the loop, you'll _mostly_ (*) get a compiler warning > about it not being initialized. > > Linus > > (*) Unless somebody initializes the iterator pointer pointlessly. > Which clearly does happen. Thus the "mostly". It's not perfect, and > that's most definitely not nice - but it should at least hopefully > make it that much harder to mess up. This won't help the current issue (because it doesn't exist and might never), but just in case some compiler people are listening, I'd like to have some sort of way to tell the compiler "treat this variable as uninitialized from here on". So one could do #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0) with __magic_uninit being a magic no-op that doesn't affect the semantics of the code, but could be used by the compiler's "[is/may be] used uninitialized" machinery to flag e.g. double frees on some odd error path etc. It would probably only work for local automatic variables, but it should be possible to just ignore the hint if p is some expression like foo->bar or has side effects. If we had that, the end-of-loop test could include that to "uninitialize" the iterator. Maybe sparse/smatch or some other static analyzer could implement such a magic thing? Maybe it's better as a function attribute [__attribute__((uninitializes(1)))] to avoid having to macrofy all functions that release resources. Rasmus
On 28/02/2022 11:08, Jakob Koschel wrote: > When list_for_each_entry() completes the iteration over the whole list > without breaking the loop, the iterator value will be a bogus pointer > computed based on the head element. > > While it is safe to use the pointer to determine if it was computed > based on the head element, either with list_entry_is_head() or > &pos->member == head, using the iterator variable after the loop should > be avoided. > > In preparation to limiting the scope of a list iterator to the list > traversal loop, use a dedicated pointer to point to the found element. > > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> [snip until i915 parts] > drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++--- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++--- > drivers/gpu/drm/i915/gt/intel_ring.c | 15 ++++--- [snip] > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 00327b750fbb..80c79028901a 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -107,25 +107,27 @@ static void lut_close(struct i915_gem_context *ctx) > radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) { > struct i915_vma *vma = rcu_dereference_raw(*slot); > struct drm_i915_gem_object *obj = vma->obj; > - struct i915_lut_handle *lut; > + struct i915_lut_handle *lut = NULL; > + struct i915_lut_handle *tmp; > > if (!kref_get_unless_zero(&obj->base.refcount)) > continue; > > spin_lock(&obj->lut_lock); > - list_for_each_entry(lut, &obj->lut_list, obj_link) { > - if (lut->ctx != ctx) > + list_for_each_entry(tmp, &obj->lut_list, obj_link) { > + if (tmp->ctx != ctx) > continue; > > - if (lut->handle != iter.index) > + if (tmp->handle != iter.index) > continue; > > - list_del(&lut->obj_link); > + list_del(&tmp->obj_link); > + lut = tmp; > break; > } > spin_unlock(&obj->lut_lock); > > - if (&lut->obj_link != &obj->lut_list) { > + if (lut) { > i915_lut_handle_free(lut); > radix_tree_iter_delete(&ctx->handles_vma, &iter, slot); Looks okay although personally I would have left lut as is for a smaller diff and introduced a new local like 'found' or 'unlinked'. > i915_vma_close(vma); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 1736efa43339..fda9e3685ad2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -2444,7 +2444,8 @@ static struct i915_request *eb_throttle(struct i915_execbuffer *eb, struct intel > { > struct intel_ring *ring = ce->ring; > struct intel_timeline *tl = ce->timeline; > - struct i915_request *rq; > + struct i915_request *rq = NULL; > + struct i915_request *tmp; > > /* > * Completely unscientific finger-in-the-air estimates for suitable > @@ -2460,15 +2461,17 @@ static struct i915_request *eb_throttle(struct i915_execbuffer *eb, struct intel > * claiming our resources, but not so long that the ring completely > * drains before we can submit our next request. > */ > - list_for_each_entry(rq, &tl->requests, link) { > - if (rq->ring != ring) > + list_for_each_entry(tmp, &tl->requests, link) { > + if (tmp->ring != ring) > continue; > > - if (__intel_ring_space(rq->postfix, > - ring->emit, ring->size) > ring->size / 2) > + if (__intel_ring_space(tmp->postfix, > + ring->emit, ring->size) > ring->size / 2) { > + rq = tmp; > break; > + } > } > - if (&rq->link == &tl->requests) > + if (!rq) > return NULL; /* weird, we will check again later for real */ Alternatively, instead of break could simply do "return i915_request_get(rq);" and replace the end of the function after the loop with "return NULL;". A bit smaller diff, or at least less "spread out" over the function, so might be easier to backport stuff touching this area in the future. But looks correct as is. > > return i915_request_get(rq); > diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c b/drivers/gpu/drm/i915/gt/intel_ring.c > index 2fdd52b62092..4881c4e0c407 100644 > --- a/drivers/gpu/drm/i915/gt/intel_ring.c > +++ b/drivers/gpu/drm/i915/gt/intel_ring.c > @@ -191,24 +191,27 @@ wait_for_space(struct intel_ring *ring, > struct intel_timeline *tl, > unsigned int bytes) > { > - struct i915_request *target; > + struct i915_request *target = NULL; > + struct i915_request *tmp; > long timeout; > > if (intel_ring_update_space(ring) >= bytes) > return 0; > > GEM_BUG_ON(list_empty(&tl->requests)); > - list_for_each_entry(target, &tl->requests, link) { > - if (target->ring != ring) > + list_for_each_entry(tmp, &tl->requests, link) { > + if (tmp->ring != ring) > continue; > > /* Would completion of this request free enough space? */ > - if (bytes <= __intel_ring_space(target->postfix, > - ring->emit, ring->size)) > + if (bytes <= __intel_ring_space(tmp->postfix, > + ring->emit, ring->size)) { > + target = tmp; > break; > + } > } > > - if (GEM_WARN_ON(&target->link == &tl->requests)) > + if (GEM_WARN_ON(!target)) > return -ENOSPC; > > timeout = i915_request_wait(target, Looks okay as well. Less clear here if there is a clean solution to make the diff smaller so no suggestions. I mean do I dare mention "goto found;" from inside the loop, where the break is, instead of the variable renames.. risky.. :) (And ofc "return -ENOSPC" immediately after the loop.) As a summary changes looks okay, up to you if you want to try to make the diffs smaller or not. It doesn't matter hugely really, all I have is a vague and uncertain "maybe it makes backporting of something, someday easier". So for i915 it is good either way. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> # i915 bits only Regards, Tvrtko
On Wed, Mar 02, 2022 at 12:07:04PM -0800, Kees Cook wrote: > On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote: > > This won't help the current issue (because it doesn't exist and might > > never), but just in case some compiler people are listening, I'd like to > > have some sort of way to tell the compiler "treat this variable as > > uninitialized from here on". So one could do > > > > #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0) > > > > with __magic_uninit being a magic no-op that doesn't affect the > > semantics of the code, but could be used by the compiler's "[is/may be] > > used uninitialized" machinery to flag e.g. double frees on some odd > > error path etc. It would probably only work for local automatic > > variables, but it should be possible to just ignore the hint if p is > > some expression like foo->bar or has side effects. If we had that, the > > end-of-loop test could include that to "uninitialize" the iterator. > > I've long wanted to change kfree() to explicitly set pointers to NULL on > free. https://github.com/KSPP/linux/issues/87 You also need to be a bit careful with existing code because there are places which do things like: drivers/usb/host/r8a66597-hcd.c 424 kfree(dev); ^^^ 425 426 for (port = 0; port < r8a66597->max_root_hub; port++) { 427 if (r8a66597->root_hub[port].dev == dev) { ^^^ 428 r8a66597->root_hub[port].dev = NULL; 429 break; 430 } 431 } Printing the freed pointer in debug code is another thing people do. regards, dan carpenter
Updating this API is risky because some places rely on the old behavior and not all of them have been updated. Here are some additional places you might want to change. drivers/usb/host/uhci-q.c:466 link_async() warn: iterator used outside loop: 'pqh' drivers/infiniband/core/mad.c:968 ib_get_rmpp_segment() warn: iterator used outside loop: 'mad_send_wr->cur_seg' drivers/opp/debugfs.c:208 opp_migrate_dentry() warn: iterator used outside loop: 'new_dev' drivers/staging/greybus/audio_codec.c:602 gbcodec_mute_stream() warn: iterator used outside loop: 'module' drivers/staging/media/atomisp/pci/atomisp_acc.c:508 atomisp_acc_load_extensions() warn: iterator used outside loop: 'acc_fw' drivers/perf/thunderx2_pmu.c:814 tx2_uncore_pmu_init_dev() warn: iterator used outside loop: 'rentry' drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c:111 nvkm_control_mthd_pstate_attr() warn: iterator used outside loop: 'pstate' drivers/gpu/drm/panfrost/panfrost_mmu.c:203 panfrost_mmu_as_get() warn: iterator used outside loop: 'lru_mmu' drivers/media/usb/uvc/uvc_v4l2.c:885 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/media/usb/uvc/uvc_v4l2.c:896 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/scsi/dc395x.c:3596 device_alloc() warn: iterator used outside loop: 'p' drivers/net/ethernet/mellanox/mlx4/alloc.c:379 __mlx4_alloc_from_zone() warn: iterator used outside loop: 'curr_node' fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start() warn: iterator used outside loop: 'res' This patchset fixes 3 bugs. Initially when it's merged it's probably going to introduce some bugs because there are likely other places which rely on the old behavior. In an ideal world, with the new API the compiler would warn about uninitialized variables, but unfortunately that warning is disabled by default so we still have to rely on kbuild/Clang/Smatch to find the bugs. But hopefully the new API encourages people to write clearer code so it prevents bugs in the long run. regards, dan carpenter
From: Dan Carpenter > Sent: 07 March 2022 15:01 > > Updating this API is risky because some places rely on the old behavior > and not all of them have been updated. Here are some additional places > you might want to change. I really can't help thinking that trying to merge this patch is actually impossible. It affects far too many different parts of the tree. Since (I believe) this is a doubly linked list with forwards and backwards pointers that point to a 'node' (not that there is a nice comment to that effect in the header - and there are lots of ways to do linked lists) the 'head' pretty much has to be a 'node'. I'd write the following new defines (but I might be using the old names here): list_first(head, field) First item, NULL if empty. list_last(head, field) Last item NULL if empty. list_next(head, item, field) Item after 'item', NULL if last. list_prev(head, item. field) Item before 'item', NULL if first. You get (something like): #define list_first(head, field) \ head->next == &head ? NULL : list_item(head->next, field) (probably needs typeof(item) from somewhere). The iterator loop is then just: #define loop_iterate(item, head, field) \ for (item = list_first(head, field); item; \ item = list_next(head, item, field) I'm not sure, but making the 'head' be a structure that contains a single member that is a 'node' might help type checking. Then all the code that uses the current defines can slowly be moved over (probably a couple of releases) before the existing defines are deleted. That should simplify all the open-coded search loops that are just as likely to be buggy (possibly more so). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
This is the first patch removing several categories of use cases of the list iterator variable past the loop. This is follow up to the discussion in: https://lore.kernel.org/all/20220217184829.1991035-1-jakobkoschel@gmail.com/ As concluded in: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/ the correct use should be using a separate variable after the loop and using a 'tmp' variable as the list iterator. The list iterator will not point to a valid structure after the loop if no break/goto was hit. Invalid uses of the list iterator variable can be avoided altogether by simply using a separate pointer to iterate the list. Linus and Greg agreed on the following pattern: - struct gr_request *req; + struct gr_request *req = NULL; + struct gr_request *tmp; struct gr_ep *ep; int ret = 0; - list_for_each_entry(req, &ep->queue, queue) { - if (&req->req == _req) + list_for_each_entry(tmp, &ep->queue, queue) { + if (&tmp->req == _req) { + req = tmp; break; + } } - if (&req->req != _req) { + if (!req) { ret = -EINVAL; goto out; } With gnu89 the list iterator variable cannot yet be declared within the for loop of the list iterator. Moving to a more modern version of C would allow defining the list iterator variable within the macro, limiting the scope to the loop. This avoids any incorrect usage past the loop altogether. This are around 30% of the cases where the iterator variable is used past the loop (identified with a slightly modified version of use_after_iter.cocci). I've decided to split it into at a few patches separated by similar use cases. Because the output of get_maintainer.pl was too big, I included all the found lists and everyone from the previous discussion. Jakob Koschel (6): drivers: usb: remove usage of list iterator past the loop body treewide: remove using list iterator after loop body as a ptr treewide: fix incorrect use to determine if list is empty drivers: remove unnecessary use of list iterator variable treewide: remove dereference of list iterator after loop body treewide: remove check of list iterator against head past the loop body arch/arm/mach-mmp/sram.c | 9 ++-- arch/arm/plat-pxa/ssp.c | 28 +++++------- arch/powerpc/sysdev/fsl_gtm.c | 4 +- arch/x86/kernel/cpu/sgx/encl.c | 6 ++- drivers/block/drbd/drbd_req.c | 45 ++++++++++++------- drivers/counter/counter-chrdev.c | 26 ++++++----- drivers/crypto/cavium/nitrox/nitrox_main.c | 11 +++-- drivers/dma/dw-edma/dw-edma-core.c | 4 +- drivers/dma/ppc4xx/adma.c | 11 +++-- drivers/firewire/core-transaction.c | 32 +++++++------ drivers/firewire/sbp2.c | 14 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 19 +++++--- drivers/gpu/drm/drm_memory.c | 15 ++++--- drivers/gpu/drm/drm_mm.c | 17 ++++--- drivers/gpu/drm/drm_vm.c | 13 +++--- drivers/gpu/drm/gma500/oaktrail_lvds.c | 9 ++-- drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++--- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++--- drivers/gpu/drm/i915/gt/intel_ring.c | 15 ++++--- .../gpu/drm/nouveau/nvkm/subdev/clk/base.c | 11 +++-- .../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++--- drivers/gpu/drm/scheduler/sched_main.c | 14 +++--- drivers/gpu/drm/ttm/ttm_bo.c | 19 ++++---- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +++++---- drivers/infiniband/hw/hfi1/tid_rdma.c | 16 ++++--- drivers/infiniband/hw/mlx4/main.c | 12 ++--- drivers/media/dvb-frontends/mxl5xx.c | 11 +++-- drivers/media/pci/saa7134/saa7134-alsa.c | 4 +- drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++++++------ drivers/misc/mei/interrupt.c | 12 ++--- .../net/ethernet/intel/i40e/i40e_ethtool.c | 3 +- .../net/ethernet/qlogic/qede/qede_filter.c | 11 +++-- drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +- .../net/wireless/intel/ipw2x00/libipw_rx.c | 15 ++++--- drivers/perf/xgene_pmu.c | 13 +++--- drivers/power/supply/cpcap-battery.c | 11 +++-- drivers/scsi/lpfc/lpfc_bsg.c | 16 ++++--- drivers/scsi/scsi_transport_sas.c | 17 ++++--- drivers/scsi/wd719x.c | 12 +++-- drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++---- drivers/staging/rtl8192e/rtllib_rx.c | 17 ++++--- .../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 ++++--- .../rtl8192u/ieee80211/rtl819x_TSProc.c | 19 ++++---- drivers/thermal/thermal_core.c | 38 ++++++++++------ drivers/usb/gadget/composite.c | 9 ++-- drivers/usb/gadget/configfs.c | 22 +++++---- drivers/usb/gadget/udc/aspeed-vhub/epn.c | 11 +++-- drivers/usb/gadget/udc/at91_udc.c | 26 ++++++----- drivers/usb/gadget/udc/atmel_usba_udc.c | 11 +++-- drivers/usb/gadget/udc/bdc/bdc_ep.c | 11 +++-- drivers/usb/gadget/udc/fsl_qe_udc.c | 11 +++-- drivers/usb/gadget/udc/fsl_udc_core.c | 11 +++-- drivers/usb/gadget/udc/goku_udc.c | 11 +++-- drivers/usb/gadget/udc/gr_udc.c | 11 +++-- drivers/usb/gadget/udc/lpc32xx_udc.c | 11 +++-- drivers/usb/gadget/udc/max3420_udc.c | 11 +++-- drivers/usb/gadget/udc/mv_u3d_core.c | 11 +++-- drivers/usb/gadget/udc/mv_udc_core.c | 11 +++-- drivers/usb/gadget/udc/net2272.c | 12 ++--- drivers/usb/gadget/udc/net2280.c | 11 +++-- drivers/usb/gadget/udc/omap_udc.c | 11 +++-- drivers/usb/gadget/udc/pxa25x_udc.c | 11 +++-- drivers/usb/gadget/udc/s3c-hsudc.c | 11 +++-- drivers/usb/gadget/udc/tegra-xudc.c | 11 +++-- drivers/usb/gadget/udc/udc-xilinx.c | 11 +++-- drivers/usb/mtu3/mtu3_gadget.c | 11 +++-- drivers/usb/musb/musb_gadget.c | 11 +++-- drivers/vfio/mdev/mdev_core.c | 11 +++-- fs/cifs/smb2misc.c | 10 +++-- fs/f2fs/segment.c | 9 ++-- fs/proc/kcore.c | 13 +++--- kernel/debug/kdb/kdb_main.c | 36 +++++++++------ kernel/power/snapshot.c | 10 +++-- kernel/trace/ftrace.c | 22 +++++---- kernel/trace/trace_eprobe.c | 15 ++++--- kernel/trace/trace_events.c | 11 ++--- net/9p/trans_xen.c | 11 +++-- net/ipv4/udp_tunnel_nic.c | 10 +++-- net/tipc/name_table.c | 11 +++-- net/tipc/socket.c | 11 +++-- net/xfrm/xfrm_ipcomp.c | 11 +++-- sound/soc/intel/catpt/pcm.c | 13 +++--- sound/soc/sprd/sprd-mcdt.c | 13 +++--- 83 files changed, 708 insertions(+), 465 deletions(-) base-commit: 7ee022567bf9e2e0b3cd92461a2f4986ecc99673 -- 2.25.1