Rasmus Villemoes
2022-Mar-02 09:29 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On 02/03/2022 00.55, Linus Torvalds wrote:> On Tue, Mar 1, 2022 at 3:19 PM David Laight <David.Laight at 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
Kees Cook
2022-Mar-02 20:07 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
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) -- Kees Cook
Linus Torvalds
2022-Mar-02 20:18 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 2, 2022 at 12:07 PM Kees Cook <keescook at chromium.org> wrote:> > I've long wanted to change kfree() to explicitly set pointers to NULL on > free. https://github.com/KSPP/linux/issues/87We'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
Dan Carpenter
2022-Mar-03 10:56 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
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