On 7/20/26 10:35, Vasileios Almpanis wrote: > > On 7/20/26 12:14 AM, Vladimir Riabchun wrote: >> >> >> On 17.07.2026 14:17, Pavel Tikhomirov wrote: >>> From: Vasileios Almpanis <[email protected]> >>> >>> In a container the per-device ve_devmnt policy restricts which options a >>> device may be mounted with and force-inserts a set of hidden options. >>> The check used to run inside the option-string parser >>> (vfs_parse_monolithic_sep) and, for remount, in a separate helper. Two >>> things escaped it: >>> >>> - MS_* flags from the legacy mount(2)/fsconfig(2) API are folded into >>> fc->sb_flags and never appear in the option string, so a container >>> could set MS_RDONLY, MS_SYNCHRONOUS, MS_MANDLOCK, ... outside its >>> allowed set. >>> >>> - The check sat in filesystem-selectable callbacks (->parse_monolithic, >>> ->mount), so a filesystem not routing through them evaded the policy, >>> and a skipped hidden-option insertion dropped a container's mandated >>> options without error. >>> >>> Enforce the policy in the fs-agnostic common mount path instead: >>> vfs_get_tree() for a new mount and reconfigure_super() for a remount. >> >> I checked and something seems uncovered: do_reconfigure_mnt via >> MS_REMOUNT|MS_BIND flags doesn't hit vfs_get_tree nor reconfigure_super, >> and yet may update flags. >> >> Am I missing something, maybe we forbid bind mounts somewhere? > > Look like you are right. Verified that we can indeed create bind mounts, in > CTs. > We can also bind mount them to another directory and then reconfigure those > bind > mounts with other options (not stated in ve.mount_opts) and it will succeed. > We > could guard bind reconfigures with capable checks otherwise we need get block > device > and go through the same checks. Before I send the next patch I'm open to > suggestions
Yes, we don't forbid bindmounts in CT. Though, those "bindmount" flags are only ro/rw, and they respect superblock ro/rw AFAIR (better we check though, let's mount ro mount and try to reconfigure it to rw). So user in CT can only make a more restrictive mount with those. So it should be ok to leave those out of scope. Also adding check for them will be assimetric with other flags. > >>> The device is taken from the mounted superblock, so fc->source cannot be >>> raced to target another device, and fc->sb_flags is vetted alongside the >>> option string. On a new mount the forced options must also be present, >>> so a filesystem that skipped inserting them has its mount refused rather >>> than silently losing them. >>> >>> The parse-time ve_devmnt_process() call is kept as a best-effort early >>> reject, so a disallowed device or option is refused before the >>> filesystem's fill_super() runs. >>> >>> https://virtuozzo.atlassian.net/browse/VSTOR-132330 >>> Fixes: 263467c864c5 ("ve/fs/devmnt: process mount options") >>> Signed-off-by: Vasileios Almpanis <[email protected]> >>> Co-developed-by: Pavel Tikhomirov <[email protected]> >>> Signed-off-by: Pavel Tikhomirov <[email protected]> >>> >>> Feature: ve: ve generic structures >>> --- >>> fs/fs_context.c | 108 +++++++++++++++++++++++++++++++++++++ >>> fs/internal.h | 1 + >>> fs/namespace.c | 97 +++++++++++++++++++++++++-------- >>> fs/super.c | 12 +++++ >>> include/linux/fs_context.h | 1 + >>> include/linux/mount.h | 1 + >>> 6 files changed, 197 insertions(+), 23 deletions(-) >>> >>> diff --git a/fs/fs_context.c b/fs/fs_context.c >>> index 76f34f3d468ea..18ffe23ad4847 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,64 @@ static inline int fscontext_lookup_bdev(struct >>> fs_context *fc, dev_t *s_dev) >>> return -ENODEV; >>> } >>> +/* >>> + * ve_devmnt_verify_fc - check a mount against the container device-mount >>> policy >>> + * @fc: the mount context, with fc->root set >>> + * @new_mount: true at vfs_get_tree() (new mount), false at >>> reconfigure_super() >>> + * >>> + * Vets the stashed option string plus the MS_* superblock flag names >>> against >>> + * the mounted superblock's device. Returns 0 when permitted (or no check >>> + * applies), or a negative errno. >>> + */ >>> +int ve_devmnt_verify_fc(struct fs_context *fc, bool new_mount) >>> +{ >>> + struct ve_struct *ve = get_exec_env(); >>> + size_t off = 0; >>> + char *page; >>> + 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; >>> + >>> + if (WARN_ON_ONCE(!fc->root)) >>> + return -EINVAL; >>> + >>> + page = (char *)__get_free_page(GFP_KERNEL); >>> + if (!page) >>> + return -ENOMEM; >>> + >>> + if (fc->ve_final_opts && *fc->ve_final_opts) { >>> + ssize_t ret = strscpy(page, fc->ve_final_opts, 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'; >>> + err = ve_devmnt_verify(ve, fc->root->d_sb->s_dev, page, new_mount); >>> + >>> +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(); >>> @@ -389,10 +482,22 @@ int vfs_parse_monolithic_sep(struct fs_context *fc, >>> void *data, >>> return -ENODEV; >>> } >>> + /* Early reject and hidden-option insertion; verified for real >>> later. */ >>> ret = ve_devmnt_process(ve, bd_dev, (void **) &options, >>> fc->purpose == FS_CONTEXT_FOR_RECONFIGURE); >>> if (ret) >>> return ret; >>> + >>> + /* Stash what the filesystem parses; checked in the common mount >>> path. */ >>> + if (options) { >>> + kfree(fc->ve_final_opts); >> >> Some file systems override parse_monolithic, for example >> smb3_fs_context_parse_monolithic. >> They won't have ve_final_opts set, so checks may fail. > Agree with this, ve_devmnt_verify_fc in my opinion should bail in case > fc->ve_final_opts == NULL, > what do you think? Yes if we have fc->ve_final_opts == NULL we should just fail immediately instead of trying to check options. For some reason I was confused that NULL there means "no options" and if we don't have any "hidden" for this bdev we can allow it, but I now thing NULL means "we don't know what options were there" so we should fail it. >> >>> + fc->ve_final_opts = kstrdup(options, GFP_KERNEL); >>> + if (!fc->ve_final_opts) { >>> + if (options != options_orig) >>> + free_page((unsigned long)options); >>> + return -ENOMEM; >>> + } >>> + } >>> } >>> /* >>> @@ -742,6 +847,7 @@ void put_fs_context(struct fs_context *fc) >>> put_filesystem(fc->fs_type); >>> if (fc->lazy_opts) >>> free_page((unsigned long)fc->lazy_opts); >>> + kfree(fc->ve_final_opts); >>> kfree(fc->source); >>> kfree(fc); >>> } >>> @@ -962,6 +1068,8 @@ void vfs_clean_context(struct fs_context *fc) >>> free_page((unsigned long)fc->lazy_opts); >>> fc->lazy_opts = NULL; >>> } >>> + kfree(fc->ve_final_opts); >>> + fc->ve_final_opts = NULL; >>> kfree(fc->source); >>> fc->source = NULL; >>> fc->exclusive = false; >>> diff --git a/fs/internal.h b/fs/internal.h >>> index 3647ce69b2c7a..e33d3ae70ccee 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_devmnt_verify_fc(struct fs_context *fc, bool new_mount); >>> 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 0f4a3668e558d..3a457588e96d9 100644 >>> --- a/fs/namespace.c >>> +++ b/fs/namespace.c >>> @@ -3258,6 +3258,79 @@ int ve_devmnt_process(struct ve_struct *ve, dev_t >>> dev, void **data_pp, int remou >>> return err; >>> } >>> +/* Return 0 if every option in @options is listed in @a or @b, else >>> -EPERM. */ >>> +static int ve_devmnt_options_subset(char *options, char *a, char *b) >> >> All these arguments should be const. > Not all, just cur and p. Those cur and p are variables, not arguments, technically. Yes, agreed, but technically "pointers to const", not "const pointers", so that we don't modify contents behind the pointers by mistake. >>> +{ >>> + char *copy, *cur, *p; >>> + int err = 0; >>> + >>> + if (!options || !*options) >>> + return 0; >>> + if (!a && !b) >>> + return -EPERM; >>> + >>> + copy = cur = kstrdup(options, GFP_KERNEL); >>> + if (!copy) >>> + return -ENOMEM; >>> + >>> + while ((p = strsep(&cur, ",")) != NULL) { >>> + if (!*p) >>> + continue; >>> + if ((!a || !strstr_separated(a, p, ',')) && >>> + (!b || !strstr_separated(b, p, ','))) { >>> + err = -EPERM; >>> + break; >>> + } >>> + } >>> + >>> + kfree(copy); >>> + return err; >>> +} >>> + >>> +/* >>> + * ve_devmnt_verify - enforce the container device-mount policy for @dev >>> + * @ve: the container >>> + * @dev: device taken from the mounted superblock (not from a raceable >>> path) >>> + * @opts: mount options plus the MS_* flag names to vet >>> + * @new_mount: true for a new mount, false for a remount >>> + * >>> + * Every supplied option must be allowed or forced. On a new mount the >>> forced >>> + * ("hidden") options must also be present: a filesystem that skipped >>> inserting >>> + * them is refused rather than silently dropping a container's mandated >>> option. >>> + */ >>> +int ve_devmnt_verify(struct ve_struct *ve, dev_t dev, char *opts, bool >>> new_mount) >>> +{ >>> + struct ve_devmnt *devmnt; >>> + char *allowed = NULL, *hidden = NULL; >>> + int err; >>> + >>> + if (ve->is_pseudosuper) >>> + return 0; >>> + >>> + mutex_lock(&ve->devmnt_mutex); >>> + list_for_each_entry(devmnt, &ve->devmnt_list, link) { >>> + if (devmnt->dev == dev) { >>> + allowed = devmnt->allowed_options; >>> + hidden = devmnt->hidden_options; >>> + break; >>> + } >>> + } >> >> What if the device is not in devmnt_list and user passed no options? >> opts is empty and allowed and hidden are NULLs, so all the following >> checks pass. > This is correct and consistent with what happens in ve_devmnt_process. Look > at commit > e7176b8050be ("fs: namespace: allow mounting blockdevices without extra > options") > For mount tries were device is not in device list (allowed and hidden doesn't > exist) the check passes Yes, we decided to allow empty opts. >> >>> + >>> + /* every supplied option must be either allowed or forced */ >>> + err = ve_devmnt_options_subset(opts, allowed, hidden); >>> + >>> + /* on a new mount every forced option must have reached the filesystem >>> */ >>> + if (!err && new_mount) >>> + err = ve_devmnt_options_subset(hidden, opts, NULL); >>> + mutex_unlock(&ve->devmnt_mutex); >>> + >>> + if (err == -EPERM) >>> + ve_pr_warn_ratelimited(VE_LOG_BOTH, "VE%s: mount options not " >>> + "permitted for device %u:%u\n", >>> + ve_name(ve), MAJOR(dev), MINOR(dev)); >> >> Is there a way to know that we lack of hidden mount options? >> With this approach, they are not hidden anymore, they are equal to >> they are required now. >> >> And with left ve_devmnt_process "user should provide these flags" is >> not working properly - in do_new_mount parse_monolithic_mount_data goes >> before vfs_get_tree, so ve_devmnt_process will add hidden_options >> instead of user. >> >> Maybe we should rename it to required_options or keep them as actually >> hidden? > > Hidden options are actually hidden. They are added by the container creator > and are inserted > to the user-passed options in ve_devmnt_insert. This mean that if hidden > options is "nodev,nosuid" > and container tries to mount this block device with "rw,relatime" then the > new mount will be > "rw,relatime,nodev,nosuid" so the name is consistent with the purpose. Yeh, hidden means "forced" here, probably not the best naming, but I'd rather leave it, because AFAIR with balloon_ino hidden option we explicitly relied that it is both forced on mounts and also not visible in container mountinfo (when read from container). > >>> + return err; >>> +} >>> + >>> static inline int ve_mount_allowed(void) >>> { >>> struct ve_struct *ve = get_exec_env(); >>> @@ -3308,23 +3381,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,12 +3413,6 @@ 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 = parse_monolithic_mount_data(fc, data); >>> if (!err) { >>> down_write(&sb->s_umount); >>> @@ -3816,6 +3866,7 @@ 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)); >>> + /* Container device-mount policy is enforced later, in vfs_get_tree(). >>> */ >>> if (!err) >>> err = parse_monolithic_mount_data(fc, data); >>> if (!err && !mount_capable(fc)) >>> diff --git a/fs/super.c b/fs/super.c >>> index 1adebbf358032..c0c067eb2d8e1 100644 >>> --- a/fs/super.c >>> +++ b/fs/super.c >>> @@ -1085,6 +1085,11 @@ int reconfigure_super(struct fs_context *fc) >>> if (retval) >>> return retval; >>> + /* Enforce the container device-mount policy on the remount options. >>> */ >>> + retval = ve_devmnt_verify_fc(fc, false); >>> + if (retval) >>> + return retval; >>> + >>> if (fc->sb_flags_mask & SB_RDONLY) { >>> #ifdef CONFIG_BLOCK >>> if (!(fc->sb_flags & SB_RDONLY) && sb->s_bdev && >>> @@ -1924,6 +1929,13 @@ int vfs_get_tree(struct fs_context *fc) >>> return error; >>> } >>> + /* Enforce the container device-mount policy against the real >>> device. */ >>> + error = ve_devmnt_verify_fc(fc, true); >>> + if (unlikely(error)) { >>> + fc_drop_locked(fc); >>> + return error; >>> + } >>> + >>> /* >>> * filesystems should never set s_maxbytes larger than >>> MAX_LFS_FILESIZE >>> * but s_maxbytes was an unsigned long long for many releases. Throw >>> diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h >>> index 1801aed1da67c..3dabb5ba11511 100644 >>> --- a/include/linux/fs_context.h >>> +++ b/include/linux/fs_context.h >>> @@ -93,6 +93,7 @@ struct fs_context { >>> struct file_system_type *fs_type; >>> void *fs_private; /* The filesystem's context */ >>> void *lazy_opts; /* mount options which can't be >>> checked at fsconfig() time */ >>> + char *ve_final_opts; /* option string handed to the fs, >>> for the ve_devmnt policy check */ >>> void *sget_key; >>> struct dentry *root; /* The root and superblock */ >>> struct user_namespace *user_ns; /* The user namespace for this >>> mount */ >>> diff --git a/include/linux/mount.h b/include/linux/mount.h >>> index 0cbc6f6893c01..12898c96cc768 100644 >>> --- a/include/linux/mount.h >>> +++ b/include/linux/mount.h >>> @@ -127,5 +127,6 @@ extern int cifs_root_data(char **dev, char **opts); >>> struct ve_struct; >>> extern int ve_devmnt_process(struct ve_struct *, dev_t, void **, int); >>> +extern int ve_devmnt_verify(struct ve_struct *, dev_t, char *, bool); >>> #endif /* _LINUX_MOUNT_H */ >> >> -- >> Best regards, Riabchun Vladimir >> Linux Kernel Developer, Virtuozzo >> -- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo. _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
