[Devel] [PATCH v6.1 VZ10 1/2] fs: enforce container device-mount policy in the common mount path
Pavel Tikhomirov
ptikhomirov at virtuozzo.com
Mon Jul 20 13:55:05 MSK 2026
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 <vasileios.almpanis at virtuozzo.com>
>>>
>>> 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 <vasileios.almpanis at virtuozzo.com>
>>> Co-developed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
>>>
>>> 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.
More information about the Devel
mailing list