Re: [PATCH] btrfs: reject unknown mount options early

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/9/28 08:26, Anand Jain wrote:
>
>
>
> On 27/09/2023 09:13, Qu Wenruo wrote:
>> [BUG]
>> The following script would allow invalid mount options to be specified
>> (although such invalid options would just be ignored):
>>
>>   # mkfs.btrfs -f $dev
>>   # mount $dev $mnt1        <<< Successful mount expected
>>   # mount $dev $mnt2 -o junk    <<< Failed mount expected
>>   # echo $?
>>   0
>>
>> [CAUSE]
>> For the 2nd mount, since the fs is already mounted, we won't go through
>> open_ctree() thus no btrfs_parse_options(), but only through
>> btrfs_parse_subvol_options().
>>
>> However we do not treat unrecognized options from valid but irrelevant
>> options, thus those invalid options would just be ignored by
>> btrfs_parse_subvol_options().
>>
>> [FIX]
>> Add the handling for Opt_error to handle invalid options and error out,
>> while still ignore other valid options inside
>> btrfs_parse_subvol_options().
>
> As discussed, the purpose of my report was to determine whether we still
> need to return success when the 'junk' option is passed in the second
> mount. I don't recall precisely if this is intentional, perhaps to
> allow future features to remain compatible with the KAPI when
> backported to an older kernel version, or if it may not be relevant in
> this kernel version.

This is not intentional, purely a bug.

As you can see in the proper btrfs_parse_options(), we handle unknown
options correctly and reject it as expected.

Furthermore, both btrfs_parse_options() and the early version
btrfs_parse_subvol_options() are using the same tokens, even if we have
future new features, it would still be rejected by btrfs_parse_options()
for a new mount.

Thus there is really no difference here, just a pure bug that doesn't
properly distinguish unknown options from valid but irrelevant options.

If you're still unsure, you can try the same thing with EXT4/XFS, they
properly handle the situation correctly, and I see no reason why we
should not.

Thanks,
Qu
>
> Thanks, Anand
>
>
>>
>> Reported-by: Anand Jain <anand.jain@oracle.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/super.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 5c6054f0552b..574fcff0822f 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -911,6 +911,10 @@ static int btrfs_parse_subvol_options(const char
>> *options, char **subvol_name,
>>               *subvol_objectid = subvolid;
>>               break;
>> +        case Opt_err:
>> +            btrfs_err(NULL, "unrecognized mount option '%s'", p);
>> +            error = -EINVAL;
>> +            goto out;
>>           default:
>>               break;
>>           }