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