I'm a bit sad that this check for mount options relies on fs-specific callbacks.
I'll send a v6.1 in reply with probably a better way + one more fix. Please consider it. On 7/7/26 19:25, Vasileios Almpanis wrote: > In the legacy mount(2) and fsconfig(2) callpaths userspace may pass mount > options either in the monolithic option string or as MS_* flags. The > flags are folded into fc->sb_flags and never appear in the option string, > so they escaped the per-device ve_devmnt policy check in > ve_devmnt_process(): a container could mount a device with options > (MS_RDONLY, MS_SYNCHRONOUS, MS_MANDLOCK, ...) that its allowed set does > not permit. > > Reconstruct the names of the set MS_* flags from the existing > flag -> string tables and run them through the ve_devmnt policy check. > The check (ve_devmnt_check_sb_flags()) is done in > vfs_parse_monolithic_sep(), reusing the block device it already resolves > for the option string, so: > > - it applies symmetrically to mount(2) (via parse_monolithic_mount_data) > and to fsconfig(2) (via the lazy_opts -> generic_parse_monolithic > finalization in vfs_cmd_create()/vfs_cmd_reconfigure()), because the > check reads fc->sb_flags regardless of how the flag was supplied; > > - the flags are vetted against the very same device resolution used for > the option string, so a fc->source symlink race cannot get them > validated against a different device than the one being mounted. > > The synthesized flag names are used only for the policy check. They are > never handed to the filesystem's own option parser (which never saw MS_* > as strings and would reject them) nor merged into fc->sb_flags_mask > (which would make reconfigure_super() reject a legacy remount carrying a > flag outside MS_RMT_MASK, e.g. MS_DIRSYNC). > > Legacy filesystems whose ->reconfigure never reaches > vfs_parse_monolithic_sep() are handled on the remount path by > ve_check_mount_options(), which runs the same check against the already > mounted (pinned) superblock device. > > https://virtuozzo.atlassian.net/browse/VSTOR-132330 > Fixes: 263467c864c5 ("ve/fs/devmnt: process mount options") > Signed-off-by: Vasileios Almpanis <[email protected]> > > Feature: ve: ve generic structures > --- > > Changes since v5: > - Fix use-after-free on the legacy mount API: do_new_mount() freed the > merged page right after parse_monolithic_mount_data(), but > legacy_parse_monolithic() retains the pointer and legacy_get_tree() > -> fs_type->mount() dereferences it later. The filesystem now only > ever sees the caller-owned original data. > - Fix remount with MS_DIRSYNC failing inside containers: re-parsing the > formatted "dirsync" set fc->sb_flags_mask |= SB_DIRSYNC, and > reconfigure_super() then rejected the remount with -EINVAL because > MS_DIRSYNC is not in MS_RMT_MASK. The flag names are no longer > re-parsed. > - Legacy filesystem option parsers no longer receive synthesized > "ro"/"sync"/... tokens they historically never saw. > - As a result the flag check is now genuinely symmetric across mount > and fsconfig: fsconfig reaches vfs_parse_monolithic_sep() via the > lazy_opts -> generic_parse_monolithic finalization, and the check > reads fc->sb_flags regardless of how the flag was supplied. > - Keep ve_check_mount_options() only on the remount path (do_remount), > where fc->root is set so it uses the pinned superblock device (no > fc->source re-walk) and covers legacy filesystems whose ->reconfigure > never reaches vfs_parse_monolithic_sep(). > - Replace legacy_merge_mount_data() (returned a buffer to be parsed) > with the two check-only helpers above; remove the now-redundant > ve_prepare_mount_options(). > - Add Fixes: tag; retitle and rewrite the commit message to describe > the policy-check-only approach. > > Changes since v4: > - Don't emit the negative/clear sb flag names (rw, async, nomand, > nolazytime) in vfs_format_sb_flags(); emit only the positive names > (common_set_sb_flag). On the legacy remount path sb_flags_mask is the > fixed MS_RMT_MASK, so the previous code appended the clear-names for > every unset remountable bit, and ve_devmnt_check() then rejected > ordinary remounts within the container not on the host(ve0). > - Fix uninitialized mount-options page: NUL-terminate the buffer before > returning it from legacy_merge_mount_data(). When data is empty/NULL > and no flags are emitted, off stayed 0 and the page from > __get_free_page() was returned non-terminated. > - Fix __vfs_format_flags() comment: it returns -E2BIG, not -ENOSPC. > > Changes since v3: > - Drop excess length check in legacy_merge_mount_data > > Changes since v2: > - Remove legacy_merge_mount_data guard in fs/internal.h. All helpers > don't use anything that would break build and just unchanged pointer > will be returned. > - Add __vfs_format_flags helper and use it in vfs_format_sb_flags > - Fix inconsistent error code: replace -ENOSPC with -E2BIG for the > buffer-too-small case > > Changes since v1: > - Replace open-coded flag loops with append_entry() helper (pointer- > advancing style) that unifies the comma-insert-copy pattern across > all three call sites > - Rework legacy_merge_mount_data() to allocate the page upfront, write > user data first then append sb flags via vfs_format_sb_flags(); this > eliminates the intermediate flags_buf[128], the total size calculation, > and the +1/+2 arithmetic > - Fix inconsistent error code: replace -EINVAL with -ENOSPC for the > buffer-too-small case > - Add comment on FS_BINARY_MOUNTDATA explaining why those filesystems > are skipped > - Add blank line before return in legacy_merge_mount_data() > - Remove excess blank line after parse_monolithic_mount_data() in > do_remount() > > fs/fs_context.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++++ > fs/internal.h | 1 + > fs/namespace.c | 33 ++++------ > 3 files changed, 171 insertions(+), 22 deletions(-) > > diff --git a/fs/fs_context.c b/fs/fs_context.c > index 76f34f3d468e..bf4cbc02ef6f 100644 > --- a/fs/fs_context.c > +++ b/fs/fs_context.c > @@ -81,6 +81,41 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const > char *key) > return -ENOPARAM; > } > > +/* > + * Emit, into @buff at *@off, the comma-separated names of every entry in @p > + * whose bit is set in @flags. Advances *@off past the written text. > + * Returns 0 on success or -E2BIG if the buffer is too small. > + */ > +static int __vfs_format_flags(const struct constant_table *p, unsigned int > flags, > + char *buff, size_t size, size_t *off) > +{ > + for (; p->name; p++) { > + ssize_t ret; > + > + if (!(flags & p->value)) > + continue; > + > + if (*off) { > + if (*off + 1 >= size) > + return -E2BIG; > + buff[(*off)++] = ','; > + } > + > + ret = strscpy(buff + *off, p->name, size - *off); > + if (ret < 0) > + return -E2BIG; > + *off += ret; > + } > + return 0; > +} > + > +static int vfs_format_sb_flags(struct fs_context *fc, char *buff, size_t > size, > + size_t *off) > +{ > + return __vfs_format_flags(common_set_sb_flag, fc->sb_flags, > + buff, size, off); > +} > + > /** > * vfs_parse_fs_param_source - Handle setting "source" via parameter > * @fc: The filesystem context to modify > @@ -224,6 +259,85 @@ static inline int fscontext_lookup_bdev(struct > fs_context *fc, dev_t *s_dev) > return -ENODEV; > } > > +/* > + * Legacy remount folds the MS_* mount flags into fc->sb_flags; they never > + * appear in the monolithic option string. Inside a container the per-device > + * ve_devmnt policy must still vet them, otherwise a flag such as > + * MS_RDONLY/MS_SYNCHRONOUS could carry an option the container is not > allowed > + * to set. Build "<user options>,<flag names>" and run it through the policy > + * check. > + * > + * Used from do_remount(): fc->root is already set, so > fscontext_lookup_bdev() > + * resolves to the mounted superblock's device rather than re-walking > + * fc->source, which both avoids a symlink race and covers legacy filesystems > + * whose ->reconfigure never reaches vfs_parse_monolithic_sep(). New mounts > vet > + * the flags in vfs_parse_monolithic_sep() instead, sharing the option > string's > + * device resolution. > + * > + * Returns 0 when the options are permitted (or no check applies), or a > negative > + * errno otherwise. > + */ > +int ve_check_mount_options(struct fs_context *fc, void *data) > +{ > + struct ve_struct *ve = get_exec_env(); > + size_t off = 0; > + void *options; > + char *page; > + dev_t dev; > + int err; > + > + if (ve_is_super(ve)) > + return 0; > + > + if (!fc->fs_type || !(fc->fs_type->fs_flags & FS_REQUIRES_DEV)) > + return 0; > + > + /* > + * Filesystems with binary mount data (e.g. btrfs) bypass option > + * string parsing entirely, so our checks cannot apply here. > + */ > + if (fc->fs_type->fs_flags & FS_BINARY_MOUNTDATA) > + return 0; > + > + err = fscontext_lookup_bdev(fc, &dev); > + if (err) { > + errorf(fc, "%s: Can't lookup blockdev", fc->source); > + return -ENODEV; > + } > + > + page = (char *)__get_free_page(GFP_KERNEL); > + if (!page) > + return -ENOMEM; > + > + if (data && *(char *)data) { > + ssize_t ret = strscpy(page, data, PAGE_SIZE); > + > + if (ret < 0) { > + err = -E2BIG; > + goto out; > + } > + off = ret; > + } > + > + err = vfs_format_sb_flags(fc, page, PAGE_SIZE, &off); > + if (err) > + goto out; > + > + page[off] = '\0'; > + > + /* > + * Check only: pass remount=1 so ve_devmnt_process() does not insert > + * hidden options into this throwaway buffer. The real insertion runs > + * on the original @data in the parse/mount_bdev path. > + */ > + options = page; > + err = ve_devmnt_process(ve, dev, &options, 1); > + > +out: > + free_page((unsigned long)page); > + return err; > +} > + > static int fscontext_init_lazy_opts(struct fs_context *fc) > { > struct ve_struct *ve = get_exec_env(); > @@ -358,6 +472,40 @@ int vfs_parse_fs_string(struct fs_context *fc, const > char *key, > } > EXPORT_SYMBOL(vfs_parse_fs_string); > > +/* > + * Vet, against the container's per-device ve_devmnt policy, the names of the > + * MS_* mount flags folded into fc->sb_flags. Called from > + * vfs_parse_monolithic_sep() with the @dev it already resolved, so the flags > + * are checked against the very same device as the option string. > + * > + * Check-only (remount=1): the synthesized names are never inserted into the > + * option string that the filesystem parses. > + */ > +static int ve_devmnt_check_sb_flags(struct fs_context *fc, struct ve_struct > *ve, > + dev_t dev) > +{ > + size_t off = 0; > + void *options; > + char *page; > + int err; > + > + page = (char *)__get_free_page(GFP_KERNEL); > + if (!page) > + return -ENOMEM; > + > + err = vfs_format_sb_flags(fc, page, PAGE_SIZE, &off); > + if (err || !off) /* error, or no MS_* flags to vet */ > + goto out; > + > + page[off] = '\0'; > + options = page; > + err = ve_devmnt_process(ve, dev, &options, 1); > + > +out: > + free_page((unsigned long)page); > + return err; > +} > + > /** > * vfs_parse_monolithic_sep - Parse key[=val][,key[=val]]* mount data > * @fc: The superblock configuration to fill in. > @@ -389,6 +537,17 @@ int vfs_parse_monolithic_sep(struct fs_context *fc, void > *data, > return -ENODEV; > } > > + /* > + * MS_* mount flags from the legacy API live in fc->sb_flags, > + * not in @options. Vet their names against the same @bd_dev > + * used for the option string below, so a fc->source symlink > + * race cannot get them validated against a different device > + * than the one being mounted. > + */ > + ret = ve_devmnt_check_sb_flags(fc, ve, bd_dev); > + if (ret) > + return ret; > + > ret = ve_devmnt_process(ve, bd_dev, (void **) &options, > fc->purpose == FS_CONTEXT_FOR_RECONFIGURE); > if (ret) > diff --git a/fs/internal.h b/fs/internal.h > index 1b5cb1cda2e4..0871265c111a 100644 > --- a/fs/internal.h > +++ b/fs/internal.h > @@ -46,6 +46,7 @@ extern void __init chrdev_init(void); > */ > extern const struct fs_context_operations legacy_fs_context_ops; > extern int parse_monolithic_mount_data(struct fs_context *, void *); > +extern int ve_check_mount_options(struct fs_context *fc, void *data); > extern void vfs_clean_context(struct fs_context *fc); > extern int finish_clean_context(struct fs_context *fc); > > diff --git a/fs/namespace.c b/fs/namespace.c > index 566f11a222fc..1de6a8a61410 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -3308,23 +3308,6 @@ static inline void ve_mount_nr_inc(struct mount *mnt, > struct ve_struct *ve) { } > static inline void ve_mount_nr_dec(struct mount *mnt) { } > #endif /* CONFIG_VE */ > > -static int ve_prepare_mount_options(struct fs_context *fc, void *data) > -{ > -#ifdef CONFIG_VE > - struct super_block *sb = fc->root->d_sb; > - struct ve_struct *ve = get_exec_env(); > - > - if (sb->s_bdev && data && !ve_is_super(ve)) { > - int err; > - > - err = ve_devmnt_process(ve, sb->s_bdev->bd_dev, &data, 1); > - if (err) > - return err; > - } > -#endif > - return 0; > -} > - > /* > * change filesystem flags. dir should be a physical root of filesystem. > * If you've mounted a non-root directory somewhere and want to do remount > @@ -3357,11 +3340,9 @@ static int do_remount(struct path *path, int ms_flags, > int sb_flags, > */ > fc->oldapi = true; > > - err = ve_prepare_mount_options(fc, data); > - if (err) { > - put_fs_context(fc); > - return err; > - } > + err = ve_check_mount_options(fc, data); > + if (err) > + goto out; > > err = parse_monolithic_mount_data(fc, data); > if (!err) { > @@ -3380,6 +3361,7 @@ static int do_remount(struct path *path, int ms_flags, > int sb_flags, > > mnt_warn_timestamp_expiry(path, &mnt->mnt); > > +out: > put_fs_context(fc); > return err; > } > @@ -3816,6 +3798,13 @@ static int do_new_mount(struct path *path, const char > *fstype, int sb_flags, > subtype, strlen(subtype)); > if (!err && name) > err = vfs_parse_fs_string(fc, "source", name, strlen(name)); > + /* > + * MS_* mount flags are vetted against the container's ve_devmnt policy > + * inside parse_monolithic_mount_data() -> vfs_parse_monolithic_sep(), > + * where they share the block-device resolution used for the option > + * string. Doing it there (rather than with a separate bdev lookup > here) > + * closes a fc->source symlink race on the flag check. > + */ > if (!err) > err = parse_monolithic_mount_data(fc, data); > if (!err && !mount_capable(fc)) -- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo. _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
