Re: [RFC] mm/huge_memory: prevent potential NULL pointer dereference

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

On Wed, Jul 16, 2025 at 11:10:00AM -0400, Zi Yan wrote:
> On 16 Jul 2025, at 10:58, Antonio Quartulli wrote:
> 
> > I just found this issue in the last linux-next Coverity report and it
> > caught my attention.
> > I am not familiar with this code, therefore I am sending this patch
> > as RFC because I am not 100% sure whether this is a false positive or
> > not.
> > However, it seems potentially legit to me:
> >
> > In __folio_split(), when looping over folios we dereference
> > `mapping` before ensuring it is non-NULL.
> >
> > Following code in the loop body performs such check, thus
> > suggesting that `mapping` may be NULL and accessing it
> > without any check may be dangerous.
> >
> > Add NULL check before passing it to shmem_mapping().
> >
> > Cc: Zi Yan <ziy@nvidia.com>
> > Fixes: 00527733d0dc ("mm/huge_memory: add two new (not yet used) functions for folio_split()")
> > Addresses-Coverity-ID: 1647614 ("FORWARD_NULL")
> > Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
> > ---
> >  mm/huge_memory.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 389620c65a5f..d649026db95a 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -3802,7 +3802,7 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> >
> >  			/* Some pages can be beyond EOF: drop them from cache */
> >  			if (new_folio->index >= end) {
> > -				if (shmem_mapping(mapping))
> > +				if (mapping && shmem_mapping(mapping))
> >  					nr_shmem_dropped += folio_nr_pages(new_folio);
> >  				else if (folio_test_clear_dirty(new_folio))
> >  					folio_account_cleaned(
> Hi Antonio,
> 
> Is there a way of preventing Coverity/sparse from checking certain code?
> This non-NULL mapping issue pops up every time I touch the code.
> 
> Dan Carpenter reported the issue yesterday and I explained it is no issue[1].
> The same report showed up when I added __split_unmapped_folio()
> back in February[2]
> 
> [1] https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/
> [2] https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/
> 
> I wonder how many times I need to explain this, although I appreciate
> your effort to improve kernel.

The way to disable static analysis is to wrap the line or function in
#ifndef __CHECKER__.  This is a bad option.  We could probably count
how many places do this using one set of fingers and toes.  We don't
like defines in .c files.  I hate when static analysis makes the code
messier and worse.  We always say to fix the checker and leave the code
alone.

We generally try to only report warnings once, but in this case you
changed the name of the function so it shows up as new.  All the scripts
say if you change the name of the file or the function that's a new
warning.  When I'm searching for duplicate reports on lore, I use the
name of the function as well.  Plus, I looked at the surrounding code
and everything on my screen was from Jul 7 so I assumed it was new.

The best option is to add a comment to the code.  The next best option
is to just endure the occasional false positive.

One idea might be to add a hash tag to static checker warnings/patches
so we can manually search lore.kernel.org for the filename and hash tag
before sending new bug reports/fixes.

regards,
dan carpenter