[GIT,PULL] ext4 changes for the 6.4 merge window

  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
  

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