Linus Torvalds
2022-Mar-01 23:55 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 1, 2022 at 3:19 PM David Laight <David.Laight at 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.
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