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

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