| Message ID | 20230425041838.GA150312@mit.edu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] ext4 changes for the 6.4 merge window | expand |
On Mon, Apr 24, 2023 at 9:18 PM Theodore Ts'o <tytso@mit.edu> wrote: > > Please note that after merging the mm and ext4 trees you will need to > apply the patch found here[1]. > > [1] https://lore.kernel.org/r/20230419120923.3152939-1-willy@infradead.org > > This is due to a patch in the mm tree, "mm: return an ERR_PTR from > __filemap_get_folio" changing that function to returning an ERR_PTR > instead of returning NULL on an error. Side note, itr would be wonderful if we could mark the places that return an error pointer as returning "nonnull", and catch things like this automatically at build time where people compare an error pointer to NULL. Howeder, it sadly turns out that compilers have gotten this completely wrong. gcc apparently completely screwed things up, and "nonnull" is not a warning aid, it's a "you can remove tests against NULL silently". And clang does seem to have taken the same approach with "returns_nonnull", which is really really sad, considering that apparently they got it right for "_Nonnull" for function arguments (where it's documented to cause a warning if you pass in a NULL argument, rather than cause the compiler to generate sh*t buggy code) Compiler people who think that "undefined behavior is a good way to implement optimizations" are a menace, and should be shunned. They are paste-eaters of the worst kind. Is there any chance that somebody could hit compiler people with a big clue-bat, and say "undefined behavior is not a feature, it's a bug", and try to make them grow up? Adding some clang people to the participants, since they at least seem to have *almost* gotten it right. Linus
The pull request you sent on Tue, 25 Apr 2023 00:18:38 -0400:
> https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git tags/ext4_for_linus
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0cfcde1fafc23068f57afa50faa3e69487b7cd30
Thank you!
On Wed, Apr 26, 2023 at 10:03 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Mon, Apr 24, 2023 at 9:18 PM Theodore Ts'o <tytso@mit.edu> wrote: > > > > Please note that after merging the mm and ext4 trees you will need to > > apply the patch found here[1]. > > > > [1] https://lore.kernel.org/r/20230419120923.3152939-1-willy@infradead.org > > > > This is due to a patch in the mm tree, "mm: return an ERR_PTR from > > __filemap_get_folio" changing that function to returning an ERR_PTR > > instead of returning NULL on an error. > > Side note, itr would be wonderful if we could mark the places that > return an error pointer as returning "nonnull", and catch things like > this automatically at build time where people compare an error pointer > to NULL. That's what clang's _Nonnull attribute does (with -Wnullability-extension). https://godbolt.org/z/6jo1zbMd9 But it's not toolchain portable, at the moment. Would require changes to clang to use the GNU C __attribute__ syntax, too (which I'm not against adding support for). > > Howeder, it sadly turns out that compilers have gotten this completely wrong. > > gcc apparently completely screwed things up, and "nonnull" is not a > warning aid, it's a "you can remove tests against NULL silently". > > And clang does seem to have taken the same approach with > "returns_nonnull", which is really really sad, considering that > apparently they got it right for "_Nonnull" for function arguments > (where it's documented to cause a warning if you pass in a NULL > argument, rather than cause the compiler to generate sh*t buggy code) Heh, I just had this conversation maybe within the past month with Bionic (Android's libc) developers. Yeah, the nonnull attributes != _Nonnull "attributes." (Quotes because IIUC _Nonnull doesn't use the __attribute__ GNU C extension syntax). My understanding (which may be wrong) is that nonnull is implemented for compatibility with GCC, while _Nonnull was likely implemented by Apple (my guess; did not check) (so compatibility with GNU C __attribute__ syntax probably wasn't considered in code review). https://clang.llvm.org/docs/AttributeReference.html#nullability-attributes The Bionic developers are deploying _Nonnull throughout the codebase and intentionally not using nonnull which is dangerous (a teammate used the term "Developer Hostile Behavior"). nonnull has implications on codegen, _Nonnull only affects diagnostics. https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability For examples. Works on return types, too. So _Nonnull can be used on return types rather than returns_nonnull. > > Compiler people who think that "undefined behavior is a good way to > implement optimizations" are a menace, and should be shunned. They are > paste-eaters of the worst kind. Thanks! :-* > > Is there any chance that somebody could hit compiler people with a big > clue-bat, and say "undefined behavior is not a feature, it's a bug", > and try to make them grow up? Good. I can feel your anger. Strike me down with all of your hatred, and your journey to the dark side will be complete. Your hate has made you powerful. Let the hate flow through you! > > Adding some clang people to the participants, since they at least seem > to have *almost* gotten it right. > > Linus
On Wed, Apr 26, 2023 at 10:34 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Apr 26, 2023 at 10:03 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > On Mon, Apr 24, 2023 at 9:18 PM Theodore Ts'o <tytso@mit.edu> wrote: > > > > > > Please note that after merging the mm and ext4 trees you will need to > > > apply the patch found here[1]. > > > > > > [1] https://lore.kernel.org/r/20230419120923.3152939-1-willy@infradead.org > > > > > > This is due to a patch in the mm tree, "mm: return an ERR_PTR from > > > __filemap_get_folio" changing that function to returning an ERR_PTR > > > instead of returning NULL on an error. > > > > Side note, itr would be wonderful if we could mark the places that > > return an error pointer as returning "nonnull", and catch things like > > this automatically at build time where people compare an error pointer > > to NULL. > > That's what clang's _Nonnull attribute does (with -Wnullability-extension). > https://godbolt.org/z/6jo1zbMd9 Ah sorry, I _thought_ that's how _Nonnull worked on return types. Let me dig some more and see how that's supposed to work...
On Wed, Apr 26, 2023 at 10:36 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Apr 26, 2023 at 10:34 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Wed, Apr 26, 2023 at 10:03 AM Linus Torvalds > > <torvalds@linux-foundation.org> wrote: > > > > > > On Mon, Apr 24, 2023 at 9:18 PM Theodore Ts'o <tytso@mit.edu> wrote: > > > > > > > > Please note that after merging the mm and ext4 trees you will need to > > > > apply the patch found here[1]. > > > > > > > > [1] https://lore.kernel.org/r/20230419120923.3152939-1-willy@infradead.org > > > > > > > > This is due to a patch in the mm tree, "mm: return an ERR_PTR from > > > > __filemap_get_folio" changing that function to returning an ERR_PTR > > > > instead of returning NULL on an error. > > > > > > Side note, itr would be wonderful if we could mark the places that > > > return an error pointer as returning "nonnull", and catch things like > > > this automatically at build time where people compare an error pointer > > > to NULL. > > > > That's what clang's _Nonnull attribute does (with -Wnullability-extension). > > https://godbolt.org/z/6jo1zbMd9 > > Ah sorry, I _thought_ that's how _Nonnull worked on return types. Let > me dig some more and see how that's supposed to work... I guess this is how I'd have expected _Nonnull to work on return types. https://godbolt.org/z/Yb7PdWv8q I'll check with the team to see if there's interest in expanding this check. Would be nice to have a GNU C __attribute__ syntax for this as well, for eventual portability.
On Wed, Apr 26, 2023 at 10:34 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > That's what clang's _Nonnull attribute does (with -Wnullability-extension). No, that's a warning about using it, not a warning about testing for NULL when it's there. I actually tested _Nonnull. It seems to work for arguments. But it does not work for return values. Of course, maybe there's some other magic needed, but it does seem to be sadly not working for us. > But it's not toolchain portable, at the moment. Would require changes > to clang to use the GNU C __attribute__ syntax, too (which I'm not > against adding support for). No need for using the __attribute__ syntax at all, that would _not_ be a show-stopper. While it's true that it's the common syntax, and we sometimes use it explicitly because of that, it's by no means the only syntax, and we actually tend to try to have more legible wrappers around it. So, for example, we prefer using '__weak' instead of writing '__attribute__((__weak__))'. And no, it very much doesn't have to use __attibute__ at all. We already have things like #define __diag(s) _Pragma(__diag_str(GCC diagnostic s)) so we already use other syntaxes. End result: if it actually worked, I'd happily do something like #define __return_nonnull _Nonnull in <linux/compiler-clang.h>, with then <linux/compiler-gcc.h> then just having #define __return_nonnull along with a big comment about how __attribute__((nonnull)) is horrible garbage that should never every be used. Linus
On Wed, Apr 26, 2023 at 11:11 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, Apr 26, 2023 at 10:34 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > That's what clang's _Nonnull attribute does (with -Wnullability-extension). > > No, that's a warning about using it, not a warning about testing for > NULL when it's there. > > I actually tested _Nonnull. It seems to work for arguments. But it > does not work for return values. Ah, it does do something in the callee, not the caller: https://godbolt.org/z/9dsPKGMWq But I see your point; it would be nice to flag that the comparison against NULL seems suspicious. > > Of course, maybe there's some other magic needed, but it does seem to > be sadly not working for us. > > > But it's not toolchain portable, at the moment. Would require changes > > to clang to use the GNU C __attribute__ syntax, too (which I'm not > > against adding support for). > > No need for using the __attribute__ syntax at all, that would _not_ be > a show-stopper. Ack. > > While it's true that it's the common syntax, and we sometimes use it > explicitly because of that, it's by no means the only syntax, and we > actually tend to try to have more legible wrappers around it. > > So, for example, we prefer using '__weak' instead of writing > '__attribute__((__weak__))'. > > And no, it very much doesn't have to use __attibute__ at all. We > already have things like > > #define __diag(s) _Pragma(__diag_str(GCC diagnostic s)) > > so we already use other syntaxes. > > End result: if it actually worked, I'd happily do something like > > #define __return_nonnull _Nonnull > > in <linux/compiler-clang.h>, with then <linux/compiler-gcc.h> then just having > > #define __return_nonnull > > along with a big comment about how __attribute__((nonnull)) is > horrible garbage that should never every be used. > > Linus
On Wed, Apr 26, 2023 at 11:22 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Ah, it does do something in the callee, not the caller: Ack, it does seem to have _some_ meaning for the return case, just not the one we'd be looking for as a way to find mistakes in the error-pointer case. Linus
On Wed, Apr 26, 2023 at 10:03:37AM -0700, Linus Torvalds wrote: > On Mon, Apr 24, 2023 at 9:18 PM Theodore Ts'o <tytso@mit.edu> wrote: > > > > Please note that after merging the mm and ext4 trees you will need to > > apply the patch found here[1]. > > > > [1] https://lore.kernel.org/r/20230419120923.3152939-1-willy@infradead.org > > > > This is due to a patch in the mm tree, "mm: return an ERR_PTR from > > __filemap_get_folio" changing that function to returning an ERR_PTR > > instead of returning NULL on an error. > > Side note, itr would be wonderful if we could mark the places that > return an error pointer as returning "nonnull", and catch things like > this automatically at build time where people compare an error pointer > to NULL. This feels like something smatch could catch. Adding Dan. Unfortunately, I don't know that we have any buildbots that run smatch, and most developers don't, so it'll always be an after-the-fact patch to fix it rather than "anybody using W=1" or "anybody using C=1" will catch it before it gets anywhere near a maintainer.
On Wed, Apr 26, 2023 at 12:11 PM Matthew Wilcox <willy@infradead.org> wrote: > > Unfortunately, I don't know that we have any buildbots that run smatch, > and most developers don't, so it'll always be an after-the-fact patch Yeah. The advantage of compiler warnings really is that they get caught quicker and developers will react to them much better. They might cause the code to be properly re-organized or rewritten to be much nicer, for example. The "trivial tree" kind of fixups for random other issues that get noticed separately tend to be much more about "work around issue". It might be the proper fix, of course, but it didn't end up being taken into account when writing the code, so it often ends up being just a "papering over the warning" kind of fix. Particularly since the person trying to fix the problem generally isn't the main developer of that code. Linus
On Wed, Apr 26, 2023 at 11:33 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, Apr 26, 2023 at 11:22 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > Ah, it does do something in the callee, not the caller: > > Ack, it does seem to have _some_ meaning for the return case, just not > the one we'd be looking for as a way to find mistakes in the > error-pointer case. Is this what you had in mind? ``` $ cat linus.c #define NULL ((void*)0) void * _Nonnull foo (void) { return &foo; } void bar (void) { if (foo() == NULL) // maybe should warn that foo() returns _Nonnull? bar(); } $ clang linus.c -fsyntax-only linus.c:8:15: warning: comparison of _Nonnull function call 'foo' equal to a null pointer is always false [-Wtautological-pointer-compare] if (foo() == NULL) // maybe should warn that foo() returns _Nonnull? ^ linus.c:3:1: note: return type has '_Nonnull' nullability attribute void * _Nonnull foo (void) { ^ 1 warning generated. ``` Quick PoC, obviously incomplete. ``` diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 18a0154b0041..10e405b1cf65 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3975,8 +3975,9 @@ def note_xor_used_as_pow_silence : Note< "replace expression with '%0' %select{|or use 'xor' instead of '^' }1to silence this warning">; def warn_null_pointer_compare : Warning< - "comparison of %select{address of|function|array}0 '%1' %select{not |}2" - "equal to a null pointer is always %select{true|false}2">, + "comparison of %select{address of|function|array|_Nonnull function call}0 " + "'%1' %select{not |}2equal to a null pointer is always " + "%select{true|false}2">, InGroup<TautologicalPointerCompare>; def warn_nonnull_expr_compare : Warning< "comparison of nonnull %select{function call|parameter}0 '%1' " @@ -3992,6 +3993,8 @@ def warn_address_of_reference_null_compare : Warning< "code; comparison may be assumed to always evaluate to " "%select{true|false}0">, InGroup<TautologicalUndefinedCompare>; +def note_return_type_nonnull : + Note<"return type has '_Nonnull' nullability attribute">; def note_reference_is_return_value : Note<"%0 returns a reference">; def note_pointer_declared_here : Note< diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index f66eb9fcf13d..9f6d326f5b72 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -13176,6 +13176,22 @@ static void AnalyzeImpConvsInComparison(Sema &S, BinaryOperator *E) { /// /// \param E the binary operator to check for warnings static void AnalyzeComparison(Sema &S, BinaryOperator *E) { + if (auto Call = dyn_cast<CallExpr>(E->getLHS())) { + QualType RetType = Call->getCallReturnType(S.Context); + if (std::optional<NullabilityKind> NK = RetType->getNullability()) { + if (*NK == NullabilityKind::NonNull && + E->getRHS()->isNullPointerConstant(S.Context, + Expr::NPC_ValueDependentIsNotNull)) { + std::string result; + llvm::raw_string_ostream os(result); + Call->getDirectCallee()->getNameForDiagnostic(os, S.getLangOpts(), true); + S.Diag(E->getExprLoc(), diag::warn_null_pointer_compare) << 3 << + result << true; + S.Diag(Call->getDirectCallee()->getReturnTypeSourceRange().getBegin(), + diag::note_return_type_nonnull); + } + } + } // The type the comparison is being performed in. QualType T = E->getLHS()->getType(); ```
On Wed, Apr 26, 2023 at 3:08 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Is this what you had in mind? > ``` > void * _Nonnull foo (void) > ... > void bar (void) { > if (foo() == NULL) // maybe should warn that foo() returns _Nonnull? > bar(); > ... > linus.c:8:15: warning: comparison of _Nonnull function call 'foo' > equal to a null pointer is always false Yes. HOWEVER. I suspect you will find that it gets complicated for more indirect uses, and that may be why people have punted on this. For example, let's say that you instead have void *bar(void) { return foo(); } and 'bar()' gets inlined. The obvious reaction to that is "ok, clearly the result is still _Nonnull, and should warn if it is tested. But that obvious reaction is actually completely wrong, because it may be that the real code looks something like void *bar(void) { #if CONFIG_XYZ if (somecondition) return NULL; #endif return foo(); } and the caller really *should* check for NULL - it's just that the compiler never saw that case. So only testing the direct return value of a function should warn. And even that "direct return value" is not trivial. What happens if you have something like this: void bar(void) { do_something(foo()); } and "do_something()" ends up being inlined - and checks for its argument for being NULL? Again, that "test against NULL" may well be absolutely required in that context - because *other* call-sites will pass in pointers that might be NULL. Now, I don't know how clang works internally, but I suspect based just on the size of your patch that your patch would get all of this horribly wrong. So doing a naked void *ptr = foo(); if (!ptr) ... should warn. But doing the exact same thing, except the test for NULL came in some other context that just got inlined, cannot warn. I _suspect_ that the reason clang does what it does is that this is just very complicated to do well. It sounds easy to a human, but ... Linus
On Wed, Apr 26, 2023 at 08:10:58PM +0100, Matthew Wilcox wrote: > > This feels like something smatch could catch. Adding Dan. > > Unfortunately, I don't know that we have any buildbots that run smatch, > and most developers don't, so it'll always be an after-the-fact patch > to fix it rather than "anybody using W=1" or "anybody using C=1" will > catch it before it gets anywhere near a maintainer. Well, if we can ask Mark Brown to run smatch on linux-next, we can catch most of these things quickly; in fact, this would have been *only* caught on linux-next, since in this case, we got caught out by a change in a function signature happening in one tree, and a new use of that function in another tree. Is this something that we could teach sparse to catch? - Ted
On Wed, Apr 26, 2023 at 03:07:48PM -0700, Nick Desaulniers wrote: > Is this what you had in mind? > ``` > $ cat linus.c > #define NULL ((void*)0) > > void * _Nonnull foo (void) { > return &foo; > } > > void bar (void) { > if (foo() == NULL) // maybe should warn that foo() returns _Nonnull? > bar(); > } > > $ clang linus.c -fsyntax-only > linus.c:8:15: warning: comparison of _Nonnull function call 'foo' > equal to a null pointer is always false Ideally, the warning should also fire in this case: if (!foo()) { bar(); } And of course, what if the code does this: p = foo(); if (p) { quux(); } Would these also be considered implicit comparisons to a NULL pointer? - Ted
On Wed, Apr 26, 2023 at 3:31 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, Apr 26, 2023 at 3:08 PM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > Is this what you had in mind? > > ``` > > void * _Nonnull foo (void) > > ... > > void bar (void) { > > if (foo() == NULL) // maybe should warn that foo() returns _Nonnull? > > bar(); > > ... > > linus.c:8:15: warning: comparison of _Nonnull function call 'foo' > > equal to a null pointer is always false > > Yes. > > HOWEVER. > > I suspect you will find that it gets complicated for more indirect > uses, and that may be why people have punted on this. > > For example, let's say that you instead have > > void *bar(void) { return foo(); } > > and 'bar()' gets inlined. > > The obvious reaction to that is "ok, clearly the result is still > _Nonnull, and should warn if it is tested. > > But that obvious reaction is actually completely wrong, because it may > be that the real code looks something like > > void *bar(void) { > #if CONFIG_XYZ > if (somecondition) return NULL; > #endif > return foo(); } > > and the caller really *should* check for NULL - it's just that the > compiler never saw that case. I think having a return value be conditionally _Nonnull is "garbage in; garbage out." The straightforward case would be to have `bar` be conditionally _Nonnull on the same preprocessor condition. void * #ifndef CONFIG_XYZ _Nonnull #endif bar (void) { #ifdef CONFIG_XYZ if (somecondition) return NULL; #endif return foo(); } Then code reviewers could go: "yikes; please make bar unconditionally _Nonnull, or simply don't use _Nonnull here." > > So only testing the direct return value of a function should warn. > > And even that "direct return value" is not trivial. What happens if > you have something like this: > > void bar(void) { do_something(foo()); } > > and "do_something()" ends up being inlined - and checks for its > argument for being NULL? Again, that "test against NULL" may well be > absolutely required in that context - because *other* call-sites will > pass in pointers that might be NULL. > > Now, I don't know how clang works internally, but I suspect based just > on the size of your patch that your patch would get all of this > horribly wrong. Of course, it was a quick hack. > > So doing a naked > > void *ptr = foo(); > if (!ptr) ... > > should warn. > > But doing the exact same thing, except the test for NULL came in some > other context that just got inlined, cannot warn. Thinking more about this, I really think _Nonnull should behave like a qualifier (const, restrict, volatile). So the above example would be: void * _Nonnull ptr = foo(); if (!ptr) // warning: tautology That would require changes to the variables that calls to functions that return ERR_PTR's were stored in. That simplifies the semantic analysis, since the compiler can simply look at the declaration of `ptr` and not try to see that `ptr`'s value at some point in the control flow graph was something returned from calling a _Nonnull returning function. Because _Nonnull isn't modeled as a qualifier in clang today, this doesn't warn: ``` void foo(void* _Nonnull); void *bar(void); void baz (void) { void *x = bar(); foo(x); } ``` It would be nice to say that "baz calls foo which expects its parameter to be _Nonnull, but you passed a pointer that could be null". I also wonder if casting works... Rust got this right; pretty sure they have NonNull and NonZero types. > > I _suspect_ that the reason clang does what it does is that this is > just very complicated to do well. > > It sounds easy to a human, but ... The bones are there; we could finish the damned thing if it sounds like something that would be useful/usable in the kernel? I guess the impetus is that ERR_PTR checks should be comparing against < 0 rather than == NULL, since that's a tautology?
On Fri, Apr 28, 2023 at 2:03 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > void *bar(void) { > > #if CONFIG_XYZ > > if (somecondition) return NULL; > > #endif > > return foo(); } > > > > and the caller really *should* check for NULL - it's just that the > > compiler never saw that case. > > I think having a return value be conditionally _Nonnull is "garbage > in; garbage out." No, no, you misunderstand. "foo()" is the one that is unconditionally _Nonnull. It never returns NULL. But *bar()* is not. There's no _Nonnull about 'bar()'. Never was, never will be. We are *not* looking to statically mark everything that never returns NULL as _Nonnull. Only some core helper functions. If "bar()" is a complicated function that can under some circumstances return NULL, then it's clearly not _Nonnull. It does not matter one whit that maybe in some simplified config, bar() might never return NULL. That simply does *not* make it _Nonnull. But my point is that for a *compiler*, this is not actually easy at all. Because a compiler might inline 'bar()' entirely (it's a trivial function that only calls 'foo()', after all, so it *should* be inlined. But now that 'bar()' has been inlined into some other call-site, that *other* call site will have a test for "is the result NULL?" And that other call site *should* have that test. Because it didn't call "foo()", it called "bar()". But with the inlining, the compiler will likely see just the call to foo(), and I suspect your patch would make it then complain about how the result is tested for NULL. So it would need to have that special logic of "only warn if the test is at the same level". > Thinking more about this, I really think _Nonnull should behave like a > qualifier (const, restrict, volatile). So the above example would be: > > void * _Nonnull ptr = foo(); > if (!ptr) // warning: tautology That would solve the problem, yes. But I suspect it would be very inconvenient for users. In particular, it would have made it totally pointless for the issue at hand: finding *existing* users of __filemap_get_folio() (that used to return NULL for errors), and finding the cases where the NULL check still exists now that it's no longer the right thign to do. So I think it would largely defeat the use-case. It would only warn for cases that have already been annotated. So to be useful, I think it would have to be a "does automagically the right thing" kind of feature. Linus
On Wed, Apr 26, 2023 at 08:10:58PM +0100, Matthew Wilcox wrote: > On Wed, Apr 26, 2023 at 10:03:37AM -0700, Linus Torvalds wrote: > > On Mon, Apr 24, 2023 at 9:18 PM Theodore Ts'o <tytso@mit.edu> wrote: > > > > > > Please note that after merging the mm and ext4 trees you will need to > > > apply the patch found here[1]. > > > > > > [1] https://lore.kernel.org/r/20230419120923.3152939-1-willy@infradead.org > > > > > > This is due to a patch in the mm tree, "mm: return an ERR_PTR from > > > __filemap_get_folio" changing that function to returning an ERR_PTR > > > instead of returning NULL on an error. > > > > Side note, itr would be wonderful if we could mark the places that > > return an error pointer as returning "nonnull", and catch things like > > this automatically at build time where people compare an error pointer > > to NULL. > > This feels like something smatch could catch. Adding Dan. > > Unfortunately, I don't know that we have any buildbots that run smatch, > and most developers don't, so it'll always be an after-the-fact patch > to fix it rather than "anybody using W=1" or "anybody using C=1" will > catch it before it gets anywhere near a maintainer. There is a Smatch check for this but actually, looking at it now, it's has some stupid stuff going on. I will fix it up a bit. But then you still have to build the cross function DB which takes overnight on my system. I run Smatch on linux-next every day and I would have caught this bug. There are also people running Coccinelle scripts which do the same thing. If I didn't catch it they would have. What I would like is an annotation in the comments that a Perl script could parse: Returns: Error pointers or valid pointer Returns: Error pointers, NULL or valid pointer Returns: Negative error codes or zero Returns: -EINVAL, 0, 1 Smatch or Coccinelle could easily use this. Btw, I have attached the Smatch warnings for this check. It's almost all dead code related to debugfs. The correct thing there is just to delete all the checking. There are a couple false positives related to functions which return both error pointers and NULL depending on the .config. regards, dan carpenter drivers/mtd/ubi/debug.c:228 ubi_debugfs_init() warn: 'dfs_rootdir' is an error pointer or valid drivers/i2c/busses/i2c-imx.c:1392 i2c_imx_init_recovery_info() warn: 'i2c_imx->pinctrl' is an error pointer or valid drivers/i2c/busses/i2c-at91-master.c:835 at91_init_twi_recovery_gpio() warn: 'rinfo->pinctrl' is an error pointer or valid drivers/i2c/busses/i2c-gpio.c:268 i2c_gpio_fault_injector_init() warn: 'i2c_gpio_debug_dir' is an error pointer or valid drivers/i2c/busses/i2c-gpio.c:273 i2c_gpio_fault_injector_init() warn: 'priv->debug_dir' is an error pointer or valid drivers/hwmon/pmbus/adm1266.c:343 adm1266_init_debugfs() warn: 'data->debugfs_dir' is an error pointer or valid drivers/hwmon/pmbus/ucd9000.c:515 ucd9000_init_debugfs() warn: 'data->debugfs' is an error pointer or valid drivers/iommu/tegra-smmu.c:1059 tegra_smmu_debugfs_init() warn: 'smmu->debugfs' is an error pointer or valid drivers/infiniband/hw/mlx4/srq.c:213 mlx4_ib_create_srq() warn: 'srq->umem' is an error pointer or valid drivers/block/nbd.c:1669 nbd_dev_dbg_init() warn: 'dir' is an error pointer or valid drivers/block/nbd.c:1695 nbd_dbg_init() warn: 'dbg_dir' is an error pointer or valid drivers/block/pktcdvd.c:454 pkt_debugfs_dev_new() warn: 'pd->dfs_d_root' is an error pointer or valid drivers/ntb/test/ntb_tool.c:1498 tool_setup_dbgfs() warn: 'tc->dbgfs_dir' is an error pointer or valid drivers/ntb/test/ntb_perf.c:1358 perf_setup_dbgfs() warn: 'perf->dbgfs_dir' is an error pointer or valid drivers/thermal/mediatek/lvts_thermal.c:191 lvts_debugfs_init() warn: 'lvts_td->dom_dentry' is an error pointer or valid drivers/thermal/mediatek/lvts_thermal.c:200 lvts_debugfs_init() warn: 'dentry' is an error pointer or valid drivers/spi/spi-dw-core.c:66 dw_spi_debugfs_init() warn: 'dws->debugfs' is an error pointer or valid drivers/spi/spi-hisi-kunpeng.c:172 hisi_spi_debugfs_init() warn: 'hs->debugfs' is an error pointer or valid drivers/gpu/drm/imx/lcdc/imx-lcdc.c:403 imx_lcdc_probe() warn: 'lcdc' is an error pointer or valid drivers/nvme/host/fault_inject.c:30 nvme_fault_inject_init() warn: 'parent' is an error pointer or valid drivers/regulator/core.c:5259 rdev_init_debugfs() warn: 'rdev->debugfs' is an error pointer or valid drivers/regulator/core.c:6181 regulator_init() warn: 'debugfs_root' is an error pointer or valid drivers/media/v4l2-core/v4l2-fwnode.c:1241 v4l2_fwnode_reference_parse_int_props() warn: 'fwnode' is an error pointer or valid drivers/scsi/mpt3sas/mpt3sas_debugfs.c:102 mpt3sas_init_debugfs() warn: 'mpt3sas_debugfs_root' is an error pointer or valid drivers/scsi/mpt3sas/mpt3sas_debugfs.c:127 mpt3sas_setup_debugfs() warn: 'ioc->debugfs_root' is an error pointer or valid drivers/scsi/qla2xxx/qla_dfs.c:119 qla2x00_dfs_create_rport() warn: 'fp->dfs_rport_dir' is an error pointer or valid drivers/scsi/qla2xxx/qla_dfs.c:708 qla2x00_dfs_setup() warn: 'vha->dfs_rport_root' is an error pointer or valid drivers/scsi/megaraid/megaraid_sas_debugfs.c:105 megasas_init_debugfs() warn: 'megasas_debugfs_root' is an error pointer or valid drivers/scsi/megaraid/megaraid_sas_debugfs.c:135 megasas_setup_debugfs() warn: 'instance->debugfs_root' is an error pointer or valid drivers/nfc/nfcsim.c:340 nfcsim_debugfs_init() warn: 'nfcsim_debugfs_root' is an error pointer or valid drivers/net/wireless/ath/ath5k/debug.c:985 ath5k_debug_init_device() warn: 'phydir' is an error pointer or valid drivers/net/wireless/ath/ath9k/htc_drv_debug.c:494 ath9k_htc_init_debug() warn: 'priv->debug.debugfs_phy' is an error pointer or valid drivers/net/wireless/ath/ath9k/debug.c:1423 ath9k_init_debug() warn: 'sc->debug.debugfs_phy' is an error pointer or valid drivers/net/wireless/ath/ath6kl/debug.c:1796 ath6kl_debug_init_fs() warn: 'ar->debugfs_phy' is an error pointer or valid drivers/net/wireless/marvell/mwifiex/debugfs.c:962 mwifiex_dev_debugfs_init() warn: 'priv->dfs_dev_dir' is an error pointer or valid drivers/net/wireless/mediatek/mt76/debugfs.c:112 mt76_register_debugfs_fops() warn: 'dir' is an error pointer or valid drivers/net/wireless/mediatek/mt7601u/debugfs.c:130 mt7601u_init_debugfs() warn: 'dir' is an error pointer or valid drivers/net/ethernet/brocade/bna/bnad_debugfs.c:503 bnad_debugfs_init() warn: 'bna_debugfs_root' is an error pointer or valid drivers/net/ethernet/brocade/bna/bnad_debugfs.c:515 bnad_debugfs_init() warn: 'bnad->port_debugfs_root' is an error pointer or valid drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c:338 otx2_ptp_init() warn: 'ptp_ptr->ptp_clock' is an error pointer or valid drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c:596 mvpp2_dbgfs_c2_entry_init() warn: 'c2_entry_dir' is an error pointer or valid drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c:629 mvpp2_dbgfs_flow_tbl_entry_init() warn: 'flow_tbl_entry_dir' is an error pointer or valid drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c:649 mvpp2_dbgfs_cls_init() warn: 'cls_dir' is an error pointer or valid drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c:653 mvpp2_dbgfs_cls_init() warn: 'c2_dir' is an error pointer or valid drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c:663 mvpp2_dbgfs_cls_init() warn: 'flow_tbl_dir' is an error pointer or valid drivers/net/ethernet/marvell/sky2.c:4532 sky2_debug_init() warn: 'ent' is an error pointer or valid drivers/net/ethernet/ti/am65-cpts.c:1154 am65_cpts_create() warn: 'cpts->ptp_clock' is an error pointer or valid drivers/net/ethernet/davicom/dm9000.c:1578 dm9000_probe() warn: 'pdata' is an error pointer or valid drivers/net/ethernet/intel/i40e/i40e_debugfs.c:1842 i40e_dbg_init() warn: 'i40e_dbg_root' is an error pointer or valid drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c:708 hns_mac_register_phydev() warn: 'phy' is an error pointer or valid drivers/net/mdio/mdio-xgene.c:268 xgene_enet_phy_register() warn: 'phy_dev' is an error pointer or valid drivers/mailbox/mailbox-test.c:262 mbox_test_add_debugfs() warn: 'tdev->root_debugfs_dir' is an error pointer or valid drivers/android/binder.c:6548 binder_init() warn: 'binder_debugfs_dir_entry_root' is an error pointer or valid security/apparmor/domain.c:1184 aa_change_hat() warn: 'new' is an error pointer or valid security/integrity/evm/evm_secfs.c:318 evm_init_secfs() warn: 'evm_symlink' is an error pointer or valid fs/nfs/dir.c:451 nfs_readdir_folio_get_next() warn: 'folio' is an error pointer or valid fs/nfsd/nfs4proc.c:377 nfsd4_create_file() warn: 'child' is an error pointer or valid fs/nfsd/nfs3proc.c:347 nfsd3_create_file() warn: 'child' is an error pointer or valid fs/pstore/zone.c:1222 psz_init_zones() warn: 'zone' is an error pointer or valid kernel/locking/lock_events.c:149 init_lockevent_counts() warn: 'd_counts' is an error pointer or valid lib/test_hmm.c:559 dmirror_allocate_chunk() warn: 'ptr' is an error pointer or valid lib/notifier-error-inject.c:86 err_inject_init() warn: 'notifier_err_inject_dir' is an error pointer or valid lib/error-inject.c:220 ei_debugfs_init() warn: 'dir' is an error pointer or valid mm/frontswap.c:266 init_frontswap() warn: 'root' is an error pointer or valid mm/shrinker_debug.c:133 shrinker_debugfs_scan_write() warn: 'memcg' is an error pointer or valid mm/filemap.c:3381 filemap_fault() warn: 'folio' is an error pointer or valid
The following changes since commit e8d018dd0257f744ca50a729e3d042cf2ec9da65: Linux 6.3-rc3 (2023-03-19 13:27:55 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git tags/ext4_for_linus for you to fetch changes up to 519fe1bae7e20fc4e7f179d50b6102b49980e85d: ext4: Add a uapi header for ext4 userspace APIs (2023-04-19 23:39:42 -0400) ------- Please note that after merging the mm and ext4 trees you will need to apply the patch found here[1]. [1] https://lore.kernel.org/r/20230419120923.3152939-1-willy@infradead.org This is due to a patch in the mm tree, "mm: return an ERR_PTR from __filemap_get_folio" changing that function to returning an ERR_PTR instead of returning NULL on an error. Because we have a lot of folio changes flowing in various trees, this patch can't be applied to either the ext4 or mm trees, but only *after* the two trees are merged together. There are also some minor merge conflicts again caused by other folio changes. Commit 5854356ab539 from linux-next's next-20230424 has the merge conflicts resolved as well as the patch in [1]. For more details/discussion, please see [2] and the prediction in [3], "I'm sure Linus will love it". :-P [2] https://lore.kernel.org/r/ZD6WNSKCjGGEFLB3@casper.infradead.org [3] https://lore.kernel.org/r/ZD8F3qV6eLHZpagX@casper.infradead.org ---------------------------------------------------------------- There are a number of major cleanups in ext4 this cycle: * The data=journal writepath has been significantly cleaned up and simplified, and reduces a large number of data=journal special cases by Jan Kara. * Ojaswin Muhoo has replaced linked list used to track extents that have been used for inode preallocation with a red-black tree in the multi-block allocator. This improves performance for workloads which do a large number of random allocating writes. * Thanks to Kemeng Shi for a lot of cleanup and bug fixes in the multi-block allocator. * Matthew wilcox has converted the code paths for reading and writing ext4 pages to use folios. * Jason Yan has continued to factor out ext4_fill_super() into smaller functions for improve ease of maintenance and comprehension. * Josh Triplett has created an uapi header for ext4 userspace API's. ---------------------------------------------------------------- Jan Kara (21): ext4: Update stale comment about write constraints ext4: Use nr_to_write directly in mpage_prepare_extent_to_map() ext4: Mark page for delayed dirtying only if it is pinned ext4: Don't unlock page in ext4_bio_write_page() ext4: Move page unlocking out of mpage_submit_page() ext4: Move mpage_page_done() calls after error handling ext4: Convert data=journal writeback to use ext4_writepages() ext4: Fix warnings when freezing filesystem with journaled data jdb2: Don't refuse invalidation of already invalidated buffers ext4: Mark pages with journalled data dirty ext4: Keep pages with journalled data dirty ext4: Clear dirty bit from pages without data to write ext4: Commit transaction before writing back pages in data=journal mode ext4: Drop special handling of journalled data from ext4_sync_file() ext4: Drop special handling of journalled data from extent shifting operations ext4: Fix special handling of journalled data from extent zeroing ext4: Drop special handling of journalled data from ext4_evict_inode() ext4: Drop special handling of journalled data from ext4_quota_on() ext4: Simplify handling of journalled data in ext4_bmap() ext4: Update comment in mpage_prepare_extent_to_map() Revert "ext4: Fix warnings when freezing filesystem with journaled data" Jason Yan (8): ext4: factor out ext4_hash_info_init() ext4: factor out ext4_percpu_param_init() and ext4_percpu_param_destroy() ext4: use ext4_group_desc_free() in ext4_put_super() to save some duplicated code ext4: factor out ext4_flex_groups_free() ext4: rename two functions with 'check' ext4: move s_reserved_gdt_blocks and addressable checking into ext4_check_geometry() ext4: factor out ext4_block_group_meta_init() ext4: move dax and encrypt checking into ext4_check_feature_compatibility() Josh Triplett (1): ext4: Add a uapi header for ext4 userspace APIs Kemeng Shi (33): ext4: properly handle error of ext4_init_block_bitmap in ext4_read_block_bitmap_nowait ext4: correct validation check of inode table in ext4_valid_block_bitmap ext4: call ext4_bg_num_gdb_[no]meta directly in ext4_num_base_meta_clusters ext4: remove unnecessary check in ext4_bg_num_gdb_nometa ext4: remove stale comment in ext4_init_block_bitmap ext4: stop trying to verify just initialized bitmap in ext4_read_block_bitmap_nowait ext4: improve inode table blocks counting in ext4_num_overhead_clusters ext4: remove unused group parameter in ext4_inode_bitmap_csum_verify ext4: remove unused group parameter in ext4_inode_bitmap_csum_set ext4: remove unused group parameter in ext4_block_bitmap_csum_verify ext4: remove unused group parameter in ext4_block_bitmap_csum_set ext4: set goal start correctly in ext4_mb_normalize_request ext4: allow to find by goal if EXT4_MB_HINT_GOAL_ONLY is set ext4: get correct ext4_group_info in ext4_mb_prefetch_fini ext4: correct calculation of s_mb_preallocated ext4: correct start of used group pa for debug in ext4_mb_use_group_pa ext4: protect pa->pa_free in ext4_discard_allocated_blocks ext4: add missed brelse in ext4_free_blocks_simple ext4: remove unused return value of ext4_mb_try_best_found and ext4_mb_free_metadata ext4: Remove unnecessary release when memory allocation failed in ext4_mb_init_cache ext4: remove unnecessary e4b->bd_buddy_page check in ext4_mb_load_buddy_gfp ext4: remove unnecessary check in ext4_mb_new_blocks ext4: remove dead check in mb_buddy_mark_free ext4: remove ac->ac_found > sbi->s_mb_min_to_scan dead check in ext4_mb_check_limits ext4: use best found when complex scan of group finishs ext4: remove unnecessary exit_meta_group_info tag ext4: remove unnecessary count2 in ext4_free_data_in_buddy ext4: remove unnecessary goto in ext4_mb_mark_diskspace_used ext4: remove repeat assignment to ac_f_ex ext4: remove comment code ext4_discard_preallocations ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple ext4: fix typos in mballoc ext4: avoid unnecessary pointer dereference in ext4_mb_normalize_request Matthew Wilcox (29): fs: Add FGP_WRITEBEGIN fscrypt: Add some folio helper functions ext4: Convert ext4_bio_write_page() to use a folio ext4: Convert ext4_finish_bio() to use folios ext4: Turn mpage_process_page() into mpage_process_folio() ext4: Convert mpage_submit_page() to mpage_submit_folio() ext4: Convert mpage_page_done() to mpage_folio_done() ext4: Convert ext4_bio_write_page() to ext4_bio_write_folio() ext4: Convert ext4_readpage_inline() to take a folio ext4: Convert ext4_convert_inline_data_to_extent() to use a folio ext4: Convert ext4_try_to_write_inline_data() to use a folio ext4: Convert ext4_da_convert_inline_data_to_extent() to use a folio ext4: Convert ext4_da_write_inline_data_begin() to use a folio ext4: Convert ext4_read_inline_page() to ext4_read_inline_folio() ext4: Convert ext4_write_inline_data_end() to use a folio ext4: Convert ext4_write_begin() to use a folio ext4: Convert ext4_write_end() to use a folio ext4: Use a folio in ext4_journalled_write_end() ext4: Convert ext4_journalled_zero_new_buffers() to use a folio ext4: Convert __ext4_block_zero_page_range() to use a folio ext4: Convert ext4_page_nomap_can_writeout to ext4_folio_nomap_can_writeout ext4: Use a folio in ext4_da_write_begin() ext4: Convert ext4_mpage_readpages() to work on folios ext4: Convert ext4_block_write_begin() to take a folio ext4: Use a folio in ext4_page_mkwrite() ext4: Use a folio iterator in __read_end_io() ext4: Convert mext_page_mkuptodate() to take a folio ext4: Convert pagecache_read() to use a folio ext4: Use a folio in ext4_read_merkle_tree_page Ojaswin Mujoo (9): ext4: Stop searching if PA doesn't satisfy non-extent file ext4: Refactor code related to freeing PAs ext4: Refactor code in ext4_mb_normalize_request() and ext4_mb_use_preallocated() ext4: Move overlap assert logic into a separate function ext4: Abstract out overlap fix/check logic in ext4_mb_normalize_request() ext4: Fix best extent lstart adjustment logic in ext4_mb_new_inode_pa() ext4: Convert pa->pa_inode_list and pa->pa_obj_lock into a union ext4: Use rbtrees to manage PAs instead of inode i_prealloc_list ext4: Remove the logic to trim inode PAs Theodore Ts'o (2): ext4: fix comment: "start start" -> "start" in mpage_prepare_extent_to_map() ext4: convert some BUG_ON's in mballoc to use WARN_RATELIMITED instead Tom Rix (1): ext4: remove unneeded check of nr_to_submit wuchi (1): ext4: remove useless conditional branch code Documentation/admin-guide/ext4.rst | 3 - MAINTAINERS | 1 + block/bio.c | 1 + fs/ext4/balloc.c | 124 ++++----- fs/ext4/bitmap.c | 13 +- fs/ext4/ext4.h | 114 +------- fs/ext4/extents.c | 35 +-- fs/ext4/fsync.c | 11 - fs/ext4/ialloc.c | 14 +- fs/ext4/inline.c | 171 ++++++------ fs/ext4/inode.c | 810 +++++++++++++++++++++----------------------------------- fs/ext4/mballoc.c | 691 +++++++++++++++++++++++++++++------------------ fs/ext4/mballoc.h | 17 +- fs/ext4/move_extent.c | 33 +-- fs/ext4/page-io.c | 116 ++++---- fs/ext4/readpage.c | 72 +++-- fs/ext4/resize.c | 7 +- fs/ext4/super.c | 413 +++++++++++++++-------------- fs/ext4/sysfs.c | 2 - fs/ext4/verity.c | 30 +-- fs/iomap/buffered-io.c | 2 +- fs/jbd2/transaction.c | 3 + fs/netfs/buffered_read.c | 3 +- fs/nfs/file.c | 12 +- include/linux/fscrypt.h | 21 ++ include/linux/page-flags.h | 5 - include/linux/pagemap.h | 2 + include/trace/events/ext4.h | 7 - include/uapi/linux/ext4.h | 117 ++++++++ mm/folio-compat.c | 4 +- 30 files changed, 1413 insertions(+), 1441 deletions(-) create mode 100644 include/uapi/linux/ext4.h