linux-kernel - Re: [PATCH] Revert "f2fs: remove unreachable lazytime mount option parsing"

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: <Zz5I2cdFn331_0ud@google.com>
Date: Wed, 20 Nov 2024 20:38:49 +0000
From: Jaegeuk Kim <jaegeuk@...nel.org>
To: Eric Sandeen <sandeen@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
	stable@...r.kernel.org, Daniel Rosenberg <drosen@...gle.com>
Subject: Re: [PATCH] Revert "f2fs: remove unreachable lazytime mount option
 parsing"

On 11/20, Eric Sandeen wrote:
> On 11/20/24 8:27 AM, Eric Sandeen wrote:
> > On 11/12/24 3:39 PM, Jaegeuk Kim wrote:
> >> Hi Eric,
> >>
> >> Could you please check this revert as it breaks the mount()?
> >> It seems F2FS needs to implement new mount support.
> >>
> >> Thanks,
> > 
> > I'm sorry, I missed this email. I will look into it more today.
> 
> Ok, I see that I had not considered a direct mount call passing
> the lazytime option strings. :(
> 
> Using mount(8), "lazytime" is never passed as an option all the way to f2fs,
> nor is "nolazytime" -
> 
> # mount -o loop,nolazytime f2fsfile.img mnt
> # mount | grep lazytime
> /root/f2fs-test/f2fsfile.img on /root/f2fs-test/mnt type f2fs (rw,relatime,lazytime,seclabel,background_gc=on,nogc_merge,discard,discard_unit=block,user_xattr,inline_xattr,acl,inline_data,inline_dentry,flush_merge,barrier,extent_cache,mode=adaptive,active_logs=6,alloc_mode=reuse,checkpoint_merge,fsync_mode=posix,memory=normal,errors=continue)
> 
> (note that lazytime is still set despite -o nolazytime)
> 
> when mount(8) is using the new mount API, it does do fsconfig for (no)lazytime:
> 
> fsconfig(3, FSCONFIG_SET_FLAG, "nolazytime", NULL, 0) = 0
> 
> but that is consumed by the VFS and never sent into f2fs for parsing.
> 
> And because default_options() does:
> 
> sbi->sb->s_flags |= SB_LAZYTIME;
> 
> by default, it overrides the "nolazytime" that the vfs had previously handled.
> 
> I'm fairly sure that when mount(8) was using the old mount API (long ago) it also
> did not send in the lazytime option string - it sent it as a flag instead.
> 
> However - a direct call to mount(2) /will/ pass those options all the way
> to f2fs, and parse_options() does need to handle them there or it will be rejected
> as an invalid option.
> 
> (Note that f2fs is the only filesystem that attempts to handle lazytime within
> the filesystem itself):
> 
> [linux]# grep -r \"lazytime\" fs/*/
> fs/f2fs/super.c:	{Opt_lazytime, "lazytime"},
> [linux]#
> 
> I'm not entirely sure how to untangle all this, but regressions are not acceptable,
> so please revert my commit for now.

Thanks for the explanation. At a glance, I thought it's caused that f2fs doesn't
implement fs_context_operations. We'll take a look at how to support it.

> 
> Thanks,
> -Eric
> 
> 
> > As for f2fs new mount API support, I have been struggling with it for a
> > long time, f2fs has been uniquely complex. The assumption that the superblock
> > and on-disk features are known at option parsing time makes it much more
> > difficult than most other filesystems.
> > 
> > But if there's a problem/regression with this commit, I have no objection to
> > reverting the commit for now, and I'm sorry for the error.
> > 
> > -Eric
> > 
> >> On 11/12, Jaegeuk Kim wrote:
> >>> This reverts commit 54f43a10fa257ad4af02a1d157fefef6ebcfa7dc.
> >>>
> >>> The above commit broke the lazytime mount, given
> >>>
> >>> mount("/dev/vdb", "/mnt/test", "f2fs", 0, "lazytime");
> >>>
> >>> CC: stable@...r.kernel.org # 6.11+
> >>> Signed-off-by: Daniel Rosenberg <drosen@...gle.com>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
> >>> ---
> >>>  fs/f2fs/super.c | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index 49519439b770..35c4394e4fc6 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -150,6 +150,8 @@ enum {
> >>>  	Opt_mode,
> >>>  	Opt_fault_injection,
> >>>  	Opt_fault_type,
> >>> +	Opt_lazytime,
> >>> +	Opt_nolazytime,
> >>>  	Opt_quota,
> >>>  	Opt_noquota,
> >>>  	Opt_usrquota,
> >>> @@ -226,6 +228,8 @@ static match_table_t f2fs_tokens = {
> >>>  	{Opt_mode, "mode=%s"},
> >>>  	{Opt_fault_injection, "fault_injection=%u"},
> >>>  	{Opt_fault_type, "fault_type=%u"},
> >>> +	{Opt_lazytime, "lazytime"},
> >>> +	{Opt_nolazytime, "nolazytime"},
> >>>  	{Opt_quota, "quota"},
> >>>  	{Opt_noquota, "noquota"},
> >>>  	{Opt_usrquota, "usrquota"},
> >>> @@ -922,6 +926,12 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> >>>  			f2fs_info(sbi, "fault_type options not supported");
> >>>  			break;
> >>>  #endif
> >>> +		case Opt_lazytime:
> >>> +			sb->s_flags |= SB_LAZYTIME;
> >>> +			break;
> >>> +		case Opt_nolazytime:
> >>> +			sb->s_flags &= ~SB_LAZYTIME;
> >>> +			break;
> >>>  #ifdef CONFIG_QUOTA
> >>>  		case Opt_quota:
> >>>  		case Opt_usrquota:
> >>> -- 
> >>> 2.47.0.277.g8800431eea-goog
> >>
> >