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

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

[Yi Wang]

[-- Attachment #1.1: Type: text/plain, Size: 996 bytes --]

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>
Signed-off-by: Changcheng Deng <deng.changcheng@zte.com.cn>
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
---
 mm/folio-compat.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/folio-compat.c b/mm/folio-compat.c
index 5b6ae1d..bee45e7 100644
--- a/mm/folio-compat.c
+++ b/mm/folio-compat.c
@@ -123,8 +123,9 @@ struct page *pagecache_get_page(struct address_space *mapping, pgoff_t index,
 	struct folio *folio;
 
 	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);
 }
 EXPORT_SYMBOL(pagecache_get_page);
-- 
1.8.3.1

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.