Re: [PATCH linux-next] mm/folio-compat.c: folio should not be NULL when it is referenced

Re: [PATCH linux-next] mm/folio-compat.c: folio should not be NULL when it is referenced

[Matthew Wilcox]

On Mon, Aug 23, 2021 at 08:47:35AM +0800, Yi Wang wrote:
> From: Changcheng Deng <deng.changcheng@zte.com.cn>
> 
> A bug was found by coccinelle:
> folio is NULL but dereferenced
> Therefore,added a check to make sure 'folio' is not NULL.
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>

Your robot is overzealous.  This does not dereference folio; rather it
takes the address of the page element of the folio structure.

By a strict reading of the C spec, it is not allowed.  However, GCC
(and I assume Clang) does the right thing.

>  	folio = __filemap_get_folio(mapping, index, fgp_flags, gfp);
> -	if ((fgp_flags & FGP_HEAD) || !folio || xa_is_value(folio))
> -		return &folio->page;
> +	if ((fgp_flags & FGP_HEAD) || xa_is_value(folio))
> +		if (folio != NULL)
> +			return &folio->page;
>  	return folio_file_page(folio, index);

This is definitely wrong.  Did you test it?  I bet you get a NULL
pointer dereference if you try it.

You could potentially make the case for:

	if (!folio)
		return NULL;
	if ((fgp_flags & FGP_HEAD) || xa_is_value(folio))
		return &folio->page;

but you actually have the same problem with the C spec, that unless
folio is actually a pointer to a folio, then &folio->page is
_technically_ undefined.  So it would have to be something even
more complex to be pedantically correct.

It's just not worth it.  Fix your tool.