Re: [linux-next:master 7893/8235] fs/jfs/jfs_metapage.c:245 __metapage_migrate_folio() error: we previously assumed 'mp' could be null (see line 235)

[Shivank Garg <shivankg@amd.com>]

On 5/7/2025 4:41 PM, Dan Carpenter wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head:   08710e696081d58163c8078e0e096be6d35c5fad
> commit: 39ed4d1a0e03ce3ac2145ee7ef0714c78bae9c61 [7893/8235] jfs: implement migrate_folio for jfs_metapage_aops
> config: i386-randconfig-141-20250502 (https://download.01.org/0day-ci/archive/20250507/202505071850.XaOkcCkX-lkp@intel.com/config)
> compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202505071850.XaOkcCkX-lkp@intel.com/
> 
> smatch warnings:
> fs/jfs/jfs_metapage.c:245 __metapage_migrate_folio() error: we previously assumed 'mp' could be null (see line 235)
> 
> vim +/mp +245 fs/jfs/jfs_metapage.c
> 
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  227  static int __metapage_migrate_folio(struct address_space *mapping, struct folio *dst,
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  228  				    struct folio *src, enum migrate_mode mode)
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  229  {
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  230  	struct metapage *mp;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  231  	int page_offset;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  232  	int rc;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  233  
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  234  	mp = folio_to_mp(src, 0);
> 39ed4d1a0e03ce Shivank Garg  2025-04-30 @235  	if (mp && metapage_locked(mp))
>                                                     ^^
> If mp is NULL
> 
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  236  		return -EAGAIN;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  237  
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  238  	rc = filemap_migrate_folio(mapping, dst, src, mode);
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  239  	if (rc != MIGRATEPAGE_SUCCESS)
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  240  		return rc;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  241  
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  242  	if (unlikely(insert_metapage(dst, mp)))
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  243  		return -EAGAIN;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  244  
> 39ed4d1a0e03ce Shivank Garg  2025-04-30 @245  	page_offset = mp->data - folio_address(src);
>                                                               ^^
> Then this will crash.
> 
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  246  	mp->data = folio_address(dst) + page_offset;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  247  	mp->folio = dst;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  248  	remove_metapage(src, mp);
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  249  
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  250  	return MIGRATEPAGE_SUCCESS;
> 39ed4d1a0e03ce Shivank Garg  2025-04-30  251  }
> 


Hi Dan,

This is a false positive.

In metapage_migrate_folio(), it checks if (!src->private) and only calls
__metapage_migrate_folio() if src->private is non-NULL

static int metapage_migrate_folio(struct address_space *mapping, struct folio *dst,
				  struct folio *src, enum migrate_mode mode)
{
...
	if (!src->private)
		return filemap_migrate_folio(mapping, dst, src, mode);
...
...
	return __metapage_migrate_folio(mapping, dst, src, mode);
}

Then __metapage_migrate_folio() calls mp = folio_to_mp(src, 0), which returns src->private.
Which should not be NULL as previously checked.

I think we can skip the mp != NULL check from 
> 39ed4d1a0e03ce Shivank Garg  2025-04-30 @235  	if (mp && metapage_locked(mp))
It seem redundant here.

Thanks,
Shivank