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: <ee341ea4-904c-4885-bf8d-8111f9e416b5@redhat.com>
Date: Wed, 20 Nov 2024 08:52:38 -0600
From: Eric Sandeen <sandeen@...hat.com>
To: Jaegeuk Kim <jaegeuk@...nel.org>, linux-kernel@...r.kernel.org,
 linux-f2fs-devel@...ts.sourceforge.net
Cc: stable@...r.kernel.org, Daniel Rosenberg <drosen@...gle.com>
Subject: Re: [PATCH] Revert "f2fs: remove unreachable lazytime mount option
 parsing"

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