| Message ID | 5c33940976b7f970836d8c796f92330e5072ffdc.1695777187.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: reject unknown mount options early | expand |
On Wed, Sep 27, 2023 at 10:43:15AM +0930, 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(). > > Reported-by: Anand Jain <anand.jain@oracle.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> Added to misc-next, thanks.
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. 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; > }
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; >> }
On Thu, Sep 28, 2023 at 08:42:20AM +0930, Qu Wenruo wrote: > 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. Yeah it's a bug, we need to split the mount option parsting due to device and subvoulme to preprocess some things but any invalid option at the first phase is also invalid in the second one so there's no reason to skip checking.
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; }
[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(). 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(+)