Re: [PATCH] filemap: Handle error return from __filemap_get_folio()

[Linus Torvalds <torvalds@linux-foundation.org>]

On Tue, May 9, 2023 at 12:43 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> I ran the new code last night.  There was one more folio bug (but every
> function in the call tree triggers a warning).
>
> fs/nfs/dir.c:405 nfs_readdir_folio_get_locked() warn: 'folio' is an error pointer or valid

Thanks. I fixed that one up too.

In the process I ended up grepping for the other wrappers around
__filemap_get_folio() that also return ERR_PTR's for errors.

I might have missed some - this was just manual, after all - but while
I'm sure smatch finds those NULL pointer confusions, what I *did* find
was a lack of testing the result at all.

In fs/btrfs/extent_io.c, we have

    while (index <= end_index) {
        folio = filemap_get_folio(mapping, index);
        filemap_dirty_folio(mapping, folio);

and in fs/gfs2/lops.c we have a similar case of filemap_get_folio ->
folio_wait_locked use without checking for any errors.

I assume it's probably hard to trigger errors in those paths, but it
does look dodgy.

Added some relevant people to the participants to let them know...

I wonder if smatch can figure out automatically when error pointers do
*not* work, and need checking for.

A lot of places do not need to check for them, because they just
return the error pointer further up the call chain, but anything that
then actually dereferences it should have checked for them (same as
for NULL, of course).

               Linus