Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
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);
So the solution should probably not be to change all those use cases to use more temporary variables, but rather to add a list_find_entry(..., condition) macro and consistently use that one instead.
Regards, Christian.
Linus
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, 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.
James
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 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, 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 February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley James.Bottomley@HansenPartnership.com wrote:
On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
Am 28.02.22 um 20:56 schrieb Linus Torvalds:
On Mon, Feb 28, 2022 at 4:19 AM Christian König christian.koenig@amd.com wrote:
I don't think that using the extra variable makes the code in any way more reliable or easier to read.
So I think the next step is to do the attached patch (which requires that "-std=gnu11" that was discussed in the original thread).
That will guarantee that the 'pos' parameter of list_for_each_entry() is only updated INSIDE the for_each_list_entry() loop, and can never point to the (wrongly typed) head entry.
And I would actually hope that it should actually cause compiler warnings about possibly uninitialized variables if people then use the 'pos' pointer outside the loop. Except
(a) that code in sgx/encl.c currently initializes 'tmp' to NULL for inexplicable reasons - possibly because it already expected this behavior
(b) when I remove that NULL initializer, I still don't get a warning, because we've disabled -Wno-maybe-uninitialized since it results in so many false positives.
Oh well.
Anyway, give this patch a look, and at least if it's expanded to do "(pos) = NULL" in the entry statement for the for-loop, it will avoid the HEAD type confusion that Jakob is working on. And I think in a cleaner way than the horrid games he plays.
(But it won't avoid possible CPU speculation of such type confusion. That, in my opinion, is a completely different issue)
Yes, completely agree.
I do wish we could actually poison the 'pos' value after the loop somehow - but clearly the "might be uninitialized" I was hoping for isn't the way to do it.
Anybody have any ideas?
I think we should look at the use cases why code is touching (pos) after the loop.
Just from skimming over the patches to change this and experience with the drivers/subsystems I help to maintain I think the primary pattern looks something like this:
list_for_each_entry(entry, head, member) { if (some_condition_checking(entry)) break; } do_something_with(entry);
Actually, we usually have a check to see if the loop found anything, but in that case it should something like
if (list_entry_is_head(entry, head, member)) { return with error; } do_somethin_with(entry);
Suffice? The list_entry_is_head() macro is designed to cope with the bogus entry on head problem.
Won't suffice because the end goal of this work is to limit scope of entry only to loop. Hence the need for additional variable.
Besides, there are no guarantees that people won't do_something_with(entry) without the check or won't compare entry to NULL to check if the loop finished with break or not.
James
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
Hi
2022. február 28., hétfő 23:28 keltezéssel, James Bottomley írta:
[...] 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.
It is arguably off-topic to the discussion at hand, but I think you might be thinking of something else (or maybe it was the case in an ancient version of C++) because that does not appear to be case. If it were,
for (int i ...) { ... } for (int i ...) { ... }
would have to trigger a redeclaration error, but that happens neither in C++ nor in C. The variable is also inaccessible outside the loop.
[...]
Regards, Barnabás Pőcze
On Mon, Feb 28, 2022 at 05:28:58PM -0500, James Bottomley wrote:
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.
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).
Segher
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 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
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 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
The thing stopping a trivial transformation of kfree() is:
kfree(get_some_pointer());
I would argue, though, that the above is poor form: the thing holding the pointer should be the thing freeing it, so these cases should be refactored and kfree() could do the NULLing by default.
Quoting myself in the above issue:
Without doing massive tree-wide changes, I think we need compiler support. If we had something like __builtin_is_lvalue(), we could distinguish function returns from lvalues. For example, right now a common case are things like:
kfree(get_some_ptr());
But if we could at least gain coverage of the lvalue cases, and detect them statically at compile-time, we could do:
#define __kfree_and_null(x) do { __kfree(*x); *x = NULL; } while (0) #define kfree(x) __builtin_choose_expr(__builtin_is_lvalue(x), __kfree_and_null(&(x)), __kfree(x))
Alternatively, we could do a tree-wide change of the former case (findable with Coccinelle) and change them into something like kfree_no_null() and redefine kfree() itself:
#define kfree_no_null(x) do { void *__ptr = (x); __kfree(__ptr); } while (0) #define kfree(x) do { __kfree(x); x = NULL; } while (0)
On Wed, Mar 2, 2022 at 12:07 PM Kees Cook keescook@chromium.org wrote:
I've long wanted to change kfree() to explicitly set pointers to NULL on free. https://github.com/KSPP/linux/issues/87
We've had this discussion with the gcc people in the past, and gcc actually has some support for it, but it's sadly tied to the actual function name (ie gcc has some special-casing for "free()")
See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527
for some of that discussion.
Oh, and I see some patch actually got merged since I looked there last so that you can mark "deallocator" functions, but I think it's only for the context matching, not for actually killing accesses to the pointer afterwards.
Linus
On Wed, Mar 02, 2022 at 12:18:45PM -0800, Linus Torvalds wrote:
On Wed, Mar 2, 2022 at 12:07 PM Kees Cook keescook@chromium.org wrote:
I've long wanted to change kfree() to explicitly set pointers to NULL on free. https://github.com/KSPP/linux/issues/87
We've had this discussion with the gcc people in the past, and gcc actually has some support for it, but it's sadly tied to the actual function name (ie gcc has some special-casing for "free()")
See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527for some of that discussion.
Oh, and I see some patch actually got merged since I looked there last so that you can mark "deallocator" functions, but I think it's only for the context matching, not for actually killing accesses to the pointer afterwards.
Ah! I missed that getting added in GCC 11. But yes, there it is:
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-mal...
Hah, now we may need to split __malloc from __alloc_size. ;)
I'd still like the NULL assignment behavior, though, since some things can easily avoid static analysis.
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
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)
I think this is a good idea.
Smatch can already find all the iterator used outside the loop bugs that Jakob did with a manageably small number of false positives. The problems are that: 1) It would be better to find it in the compile stage instead of later. 2) I hadn't published that check. Will do shortly. 3) A couple weeks back I noticed that the list_for_each_entry() check was no longer working. Fixed now. 4) Smatch was only looking at cases which dereferenced the iterator and not checks for NULL. I will test the fix for that tonight. 5) Smatch is broken on PowerPC.
Coccinelle also has checks for iterator used outside the loop. Coccinelle had these checks before Smatch did. I copied Julia's idea.
If your annotation was added to GCC it would solve all those problems.
But it's kind of awkward that we can't annotate kfree() directly instead of creating the kfree() macro. And there are lots of other functions which free things so you'd have to create a ton of macros like:
#define gr_free_dma_desc(a, b) do { __gr_free_dma_desc(a, b); __magic_uninit(b); } while (0)
And then there are functions which free a struct member:
void free_bar(struct foo *p) { kfree(p->bar); }
Or functions which free a container_of().
Smatch is more evolved than designed but what I do these days is use $0, $1, $2 to represent the parameters. So you can say a function frees $0->bar. For container_of() then is "(168<~$0)->bar" which means 168 bytes from $0. Returns are parameter -1 so I guess it would be $(-1), but as I said Smatch evolved so right now places that talk about returned values use a different format.
What you could do is just make a parseable table next to the function definition with all the information. Then you would use a Perl script to automatically generate a Coccinelle check to warn about use after frees.
diff --git a/mm/slab.c b/mm/slab.c index ddf5737c63d9..c9dffa5c40a2 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3771,6 +3771,9 @@ EXPORT_SYMBOL(kmem_cache_free_bulk); * * Don't free memory not originally allocated by kmalloc() * or you will run into trouble. + * + * CHECKER information + * frees: $0 */ void kfree(const void *objp) {
regards, dan carpenter
participants (12)
-
Barnabás Pőcze -
Christian König -
Dan Carpenter -
David Laight -
Jakob Koschel -
James Bottomley -
Jeffrey Walton -
Kees Cook -
Linus Torvalds -
Mike Rapoport -
Rasmus Villemoes -
Segher Boessenkool