On 6/2/26 10:14, Vasileios Almpanis wrote:
> In legacy mount callpaths, userspace might pass mount options as
> flags. These flags escape our checks in ve_devmnt_process allowing
> devices to be mounted inside containers with options not specified in
> the allowed field. Introduce helpers that take these flags and
> already existing tables of flag -> string representation to construct
> a comma separated value string from them, and append them to userspace
> provided data. Then pass this string to parse_monolithic_mount_data
> enforcing the same checks symmetrically in both mount and fsconfig
> syscalls.
>
> In the remount path, run legacy_merge_mount_data() before
> ve_devmnt_process() so container device mount policy sees MS_* flags
> from the legacy mount(2) API, not only the user-supplied option string.
> Keep ve_prepare_mount_options() for legacy parsers that do not use
> generic_parse_monolithic().
>
> https://virtuozzo.atlassian.net/browse/VSTOR-132330
> Signed-off-by: Vasileios Almpanis <[email protected]>
>
> Feature: ve: ve generic structures
> ---
> fs/fs_context.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++
> fs/internal.h | 8 ++++
> fs/namespace.c | 33 +++++++++++----
> 3 files changed, 139 insertions(+), 7 deletions(-)
>
> diff --git a/fs/fs_context.c b/fs/fs_context.c
> index 76f34f3d468e..65495421fa9e 100644
> --- a/fs/fs_context.c
> +++ b/fs/fs_context.c
> @@ -81,6 +81,111 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const
> char *key)
> return -ENOPARAM;
> }
>
> +#ifdef CONFIG_VE
> +/*
> + * Format fc->sb_flags as comma-separated mount option names (ro, sync, ...).
> + * Matches vfs_parse_sb_flag() / common_{set,clear}_sb_flag tables.
> + */
> +static int vfs_format_sb_flags(struct fs_context *fc, char *buf, size_t
> buflen)
> +{
> + const struct constant_table *p;
> + unsigned int flags = fc->sb_flags;
> + unsigned int mask = fc->sb_flags_mask;
> + size_t len = 0;
> + int token;
> +
> + for (p = common_set_sb_flag; p->name; p++) {
> + size_t namelen = strlen(p->name);
> +
> + token = p->value;
> + if (!(flags & token))
> + continue;
> + if (len + namelen + 2 > buflen)
You try to merge "," check and namelen check and also '\0' check in one, but
this way the check is not precise, if we have just one option and it is a long
one and has buflen-1 characters it can still fit into the buf but your check
would fail.
I would rather split the checks:
if (len && len < buflen)
buf[len++] = ',';
if (len + namelen > buflen)
return -ENOSPC;
...
if (len + 1 > buflen)
return -ENOSPC;
buf[len++] = '\0';
This way it is much more concrete and easier to read.
> + return -ENOSPC;
> + if (len)
> + buf[len++] = ',';
> + memcpy(buf + len, p->name, namelen);
> + len += namelen;
> + }
> +
> + for (p = common_clear_sb_flag; p->name; p++) {
> + size_t namelen = strlen(p->name);
> +
> + token = p->value;
> + if (!(mask & token) || (flags & token))
> + continue;
> + if (len + namelen + 2 > buflen)
> + return -ENOSPC;
> + if (len)
> + buf[len++] = ',';
> + memcpy(buf + len, p->name, namelen);
> + len += namelen;
> + }
Would be nice to put the loop into a helper function and call it twice instead
of twice open-coding it.
> +
> + buf[len] = '\0';
> + return len;
> +}
> +
> +/*
> + * For legacy mount(2), MS_* mount flags are folded into fc->sb_flags and are
> + * not present in the monolithic data string. Build a page holding those
> + * options plus any user data for ve_devmnt checks in
> vfs_parse_monolithic_sep.
> + *
> + * Returns @data when nothing needs to be added, a new page otherwise, or
> + * ERR_PTR() on failure. The caller must free_page() when the result !=
> @data.
> + */
> +void *legacy_merge_mount_data(struct fs_context *fc, void *data)
> +{
> + struct ve_struct *ve = get_exec_env();
> + char flags_buf[128];
> + size_t data_len = 0;
> + size_t total;
> + char *page, *p;
> + int flags_len;
> +
> + if (ve_is_super(ve))
> + return data;
> +
> + if (!fc->fs_type || !(fc->fs_type->fs_flags & FS_REQUIRES_DEV))
> + return data;
> +
> + if (fc->fs_type->fs_flags & FS_BINARY_MOUNTDATA)
> + return data;
> +
> + flags_len = vfs_format_sb_flags(fc, flags_buf, sizeof(flags_buf));
> + if (flags_len < 0)
> + return ERR_PTR(flags_len);
> +
> + if (!flags_len)
> + return data;
> +
> + if (data && *(char *)data)
> + data_len = strlen(data);
> +
> + total = flags_len + 1;
> + if (data_len)
> + total += data_len + 1;
> +
> + if (total > PAGE_SIZE)
> + return ERR_PTR(-EINVAL);
> +
> + page = (char *)__get_free_page(GFP_KERNEL);
> + if (!page)
> + return ERR_PTR(-ENOMEM);
> +
> + p = page;
> + memcpy(p, flags_buf, flags_len);
> + p += flags_len;
> + if (data_len) {
> + *p++ = ',';
> + memcpy(p, data, data_len);
> + p += data_len;
> + }
> + *p = '\0';
> + return page;
> +}
> +#endif /* CONFIG_VE */
> +
> /**
> * vfs_parse_fs_param_source - Handle setting "source" via parameter
> * @fc: The filesystem context to modify
> diff --git a/fs/internal.h b/fs/internal.h
> index 0064345cb9d0..120c239df394 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -46,6 +46,14 @@ 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 *);
> +#ifdef CONFIG_VE
> +void *legacy_merge_mount_data(struct fs_context *fc, void *data);
> +#else
> +static inline void *legacy_merge_mount_data(struct fs_context *fc, void
> *data)
> +{
> + return data;
> +}
> +#endif
> 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 acd4507e1247..27a72d4cb245 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3295,6 +3295,7 @@ static int do_remount(struct path *path, int ms_flags,
> int sb_flags,
> struct super_block *sb = path->mnt->mnt_sb;
> struct mount *mnt = real_mount(path->mnt);
> struct fs_context *fc;
> + void *mnt_data = NULL;
>
> if (!check_mnt(mnt))
> return -EINVAL;
> @@ -3315,13 +3316,18 @@ 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;
> + mnt_data = legacy_merge_mount_data(fc, data);
> + if (IS_ERR(mnt_data)) {
> + err = PTR_ERR(mnt_data);
> + goto err;
> }
>
> - err = parse_monolithic_mount_data(fc, data);
> + err = ve_prepare_mount_options(fc, mnt_data);
> + if (err)
> + goto free_mnt_data;
> +
> + err = parse_monolithic_mount_data(fc, mnt_data);
> +
> if (!err) {
> down_write(&sb->s_umount);
> err = -EPERM;
> @@ -3338,6 +3344,10 @@ static int do_remount(struct path *path, int ms_flags,
> int sb_flags,
>
> mnt_warn_timestamp_expiry(path, &mnt->mnt);
>
> +free_mnt_data:
> + if (mnt_data && mnt_data != data)
> + free_page((unsigned long)mnt_data);
> +err:
> put_fs_context(fc);
> return err;
> }
> @@ -3774,8 +3784,17 @@ 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));
> - if (!err)
> - err = parse_monolithic_mount_data(fc, data);
> + if (!err) {
> + void *mnt_data = legacy_merge_mount_data(fc, data);
> +
> + if (IS_ERR(mnt_data)) {
> + err = PTR_ERR(mnt_data);
> + } else {
> + err = parse_monolithic_mount_data(fc, mnt_data);
> + if (mnt_data != data)
> + free_page((unsigned long)mnt_data);
> + }
> + }
> if (!err && !mount_capable(fc))
> err = -EPERM;
> if (!err)
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
