linux-kernel - Re: [PATCH] f2fs: fix error path handling in f2fs_read_data_large

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2f63622e-9663-491b-b390-71e6e81474ce@kernel.org>
Date: Fri, 9 Jan 2026 09:37:54 +0800
From: Chao Yu <chao@...nel.org>
To: David Laight <david.laight.linux@...il.com>
Cc: chao@...nel.org, jaegeuk@...nel.org,
 linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] f2fs: fix error path handling in
 f2fs_read_data_large_folio()

On 1/9/2026 6:45 AM, David Laight wrote:
> On Thu,  8 Jan 2026 05:42:31 +0800
> Chao Yu <chao@...nel.org> wrote:
> 
>> In error path of f2fs_read_data_large_folio(), if bio is valid, it
>> may submit bio twice, fix it.
> 
> That isn't the only bug at the bottom of that function.
> I think I've unravelled the strange loops on the copy in linux-next.
> The 'goto got_it' could be a normal conditional.
> 
> The top has:
> 	if (rac)
> 		folio = readahead_folio(rac);
> next_folio:
> 	if (!folio)
> 		goto out:
> which means you can 'goto out' before setting up a pending 'bio'.
> 
> I'm sure that could be made a proper loop - although it would cost an indentation.
> Would certainly be better with only one call to readahead_foilio(), perhaps:
> next_folio:
> 	if (rac) {
> 		folio = readahead_folio(rac);
> 		if (!folio)
> 			goto out:
> 	}
> with just:
> 	if (rac)
> 		goto next_folio;
> at the bottom.

It could be, maybe you can clean up w/ a separated patch?

> 
> 
>>
>> Signed-off-by: Chao Yu <chao@...nel.org>
>> ---
>>   fs/f2fs/data.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index cabaeeb436bd..386d9adfd4bd 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -2568,17 +2568,14 @@ static int f2fs_read_data_large_folio(struct inode *inode,
>>   		folio_unlock(folio);
>>   		return ret;
>>   	}
>> -
>> +out:
>> +	f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
>>   	if (ret) {
>> -		f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
>> -
>>   		/* Wait bios and clear uptodate. */
>>   		folio_lock(folio);
> 
> If I've read the code correctly the 'bio' can contain transfers for a
> previous folio(s), and might have transfers for this folio, but might not.
> So relocking the folio may just deadlock.

The bio may contain last folio which has not been committed to device, as you
said the folio has been locked, and after IO completion, we will unlock the
folio in f2fs_finish_read_bio(), so no deadlock?

Thanks,

> (I've not found the unlock at the end of transfer...)
> Quite which 'bio' need the flag changed is another question.
> 
> 	David
> 
>>   		folio_clear_uptodate(folio);
>>   		folio_unlock(folio);
>>   	}
>> -out:
>> -	f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
>>   	return ret;
>>   }
>>   
>