[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)

[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)

[Dan Carpenter]

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  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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]

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

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)

[Andrew Morton]

On Wed, 7 May 2025 17:19:52 +0530 Shivank Garg <shivankg@amd.com> wrote:

> 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.

Yes.  Right now the code simply looks wrong:

	mp = folio_to_mp(src, 0);
	if (mp && metapage_locked(mp))
		return -EAGAIN;

	...

	page_offset = mp->data - folio_address(src);

any reader of this will be worried.  A code comment (at least) would
save them concern and time.  Reworking the code to eliminate doubt
would be better.

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)

[Dan Carpenter]

On Wed, May 07, 2025 at 05:19:52PM +0530, Shivank Garg wrote:
> 
> 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.

Yep.  Just delete the unnecessary NULL check.

I only consider these a false positive if the NULL check is
required but it doesn't lead to a crash.  For example, there could be
another condition which is equivalent to checking for NULL.

	if (p)
		p->foo = 100;
	...
	if (!p_valid(p))
		return;

	p->foo = 99;

In this code, if you have the cross function database then Smatch
doesn't print a warning because the NULL check is not required.  But
the cross function database is way too slow to use in the kbuild-bot.

regards,
dan carpenter

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]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]



On 5/8/2025 11:08 AM, Dan Carpenter wrote:
> On Wed, May 07, 2025 at 05:19:52PM +0530, Shivank Garg wrote:
>>
>> 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.
> 
> Yep.  Just delete the unnecessary NULL check.
> 
> I only consider these a false positive if the NULL check is
> required but it doesn't lead to a crash.  For example, there could be
> another condition which is equivalent to checking for NULL.
> 
> 	if (p)
> 		p->foo = 100;
> 	...
> 	if (!p_valid(p))
> 		return;
> 
> 	p->foo = 99;
> 
> In this code, if you have the cross function database then Smatch
> doesn't print a warning because the NULL check is not required.  But
> the cross function database is way too slow to use in the kbuild-bot.
> 
> regards,
> dan carpenter

> Yes.  Right now the code simply looks wrong:
> 
> 	mp = folio_to_mp(src, 0);
> 	if (mp && metapage_locked(mp))
> 		return -EAGAIN;
> 
> 	...
> 
> 	page_offset = mp->data - folio_address(src);
> 
> any reader of this will be worried.  A code comment (at least) would
> save them concern and time.  Reworking the code to eliminate doubt
> would be better.

Hi Andrew, Dan,

Thank you for reviewing this and confirming.
I've attached the patch for removing mp NULL check.

Best Regards,
Shivank







[-- Attachment #2: 0001-jfs-remove-redundant-NULL-check-in-__metapage_migrat.patch --]
[-- Type: text/plain, Size: 1172 bytes --]

From 37688e00ed3e8c603778ee5271bb12ac11a32249 Mon Sep 17 00:00:00 2001
From: Shivank Garg <shivankg@amd.com>
Date: Thu, 8 May 2025 06:00:33 +0000
Subject: [PATCH] jfs: remove redundant NULL check in
 __metapage_migrate_folio()

The NULL check for 'mp' is unnecessary since metapage_migrate_folio()
already verifies src->private exists before calling this function.

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/
Fixes: 708c4552aa37 ("jfs: implement migrate_folio for jfs_metapage_aops")
Signed-off-by: Shivank Garg <shivankg@amd.com>
---
 fs/jfs/jfs_metapage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
index 7e23db142b27..9029cd216912 100644
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -238,7 +238,7 @@ static int __metapage_migrate_folio(struct address_space *mapping,
 	int rc;
 
 	mp = folio_to_mp(src, 0);
-	if (mp && metapage_locked(mp))
+	if (metapage_locked(mp))
 		return -EAGAIN;
 
 	rc = filemap_migrate_folio(mapping, dst, src, mode);
-- 
2.34.1