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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260108224545.3019a411@pumpkin>
Date: Thu, 8 Jan 2026 22:45:45 +0000
From: David Laight <david.laight.linux@...il.com>
To: Chao Yu <chao@...nel.org>
Cc: 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 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. 


> 
> 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.
(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;
>  }
>