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

[Dan Carpenter <dan.carpenter@linaro.org>]

On Sat, May 06, 2023 at 10:41:22AM -0700, Andrew Morton wrote:
> --- a/fs/afs/dir_edit.c~afs-fix-the-afs_dir_get_folio-return-value
> +++ a/fs/afs/dir_edit.c
> @@ -115,11 +115,12 @@ static struct folio *afs_dir_get_folio(s
>  	folio = __filemap_get_folio(mapping, index,
>  				    FGP_LOCK | FGP_ACCESSED | FGP_CREAT,
>  				    mapping->gfp_mask);
> -	if (IS_ERR(folio))
> +	if (IS_ERR(folio)) {
>  		clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
> -	else if (folio && !folio_test_private(folio))
> +		return NULL;
> +	}
> +	if (!folio_test_private(folio))
>  		folio_attach_private(folio, (void *)1);
> -
>  	return folio;
>  }

This one is quite tricky for Smatch.  I mentioned earlier that the
existing Smatch check for error pointer dereferences has some stupid
stuff going on.  I've replaced some of the stupid and I'll testing it
tonight.

1)  There is an existing check which complains if you have "if (p) "
    where p can be an error pointer, but not NULL.  If I revert the fix,
    I get the correct warning now.

    fs/afs/dir_edit.c:242 afs_edit_dir_add()
    warn: 'folio0' is an error pointer or valid *NEW*

2) There is an existing check for dereferencing error pointers.  However,
   I don't think kmap_local_folio() actually  dereferences the folio.
   The folio_nr_pages() function does, but depending on the .config,
   it's kind of complicated and buried inside a READ_ONCE().  I've
   improved the Smatch code for this but I don't have a solution yet.

3) I've created a new warning which generally complains about passing
   error pointers.  Obviously there are functions where that's normal,
   like passing error pointers to IS_ERR() and dev_err_probe().  It
   may or may not be useful.  I'll look at the warnings tomorrow.

    fs/afs/dir_edit.c:265 afs_edit_dir_add()
    warn: passing error pointer 'folio0' to folio_nr_pages()

regards,
dan carpenter