Re: [Devel] [PATCH VZ10 v9 2/8] ve/fs: Rework per-ve mount count

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

Sashiko review:

On 9/15/26 19:22, Vladimir Riabchun wrote:
> Previous approach with current mounts counter had an issue:
> there was a gap between ve_mount_allowed check and ve_mount_nr_inc,
> which could allow CT to have more mounts than expected.
> 
> Fix this by tracking the number of available mounts instead
> of current ones. This also makes resources accounting
> more consistent - we are using ***_avail_nr approach more.
> 
> One more issue with inconsistent ve value is fixed:
> ve_mount_allowed always used ve from get_exec_env, but
> ve_mount_nr_inc operated with owner_ve.
> Now actual ve value is calculated in the beginning of alloc_vfsmnt.
> 
> To avoid incorrect accounting when is_pseudosuper is changed,
> update avail_nr count without > 0 check if VE is ve0 or pseudosuper.
> This also simplifies ve_mount_put, since increment is
> now unconditional.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-135520
> 
> Feature: per-ve failcounters
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> ---
>   fs/namespace.c     | 78 ++++++++++++++++++++++++++++------------------
>   include/linux/ve.h |  2 +-
>   kernel/ve/ve.c     | 12 +++----
>   3 files changed, 54 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 4adc1db84b4c..1d6e66cd7174 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -317,19 +317,22 @@ int mnt_get_count(struct mount *mnt)
>   #endif
>   }
>   
> -static inline int ve_mount_allowed(void);
> -static inline void ve_mount_nr_inc(struct mount *mnt, struct ve_struct *ve);
> -static inline void ve_mount_nr_dec(struct mount *mnt);
> +static inline int ve_try_reserve_mount(struct ve_struct *ve);
> +static inline void ve_mount_put(struct mount *mnt, struct ve_struct *ve);
>   static inline void ve_mount_reown(struct mount *mnt, struct ve_struct *ve);
>   
>   static struct mount *alloc_vfsmnt(const char *name, struct ve_struct *owner_ve)
>   {
>   	struct mount *mnt;
> +	struct ve_struct *ve = owner_ve;
>   
> -	if (!ve_mount_allowed()) {
> +	if (!ve)
> +		ve = get_exec_env();
> +
> +	if (!ve_try_reserve_mount(ve)) {
>   		pr_warn_ratelimited(
>   			"CT#%s reached the limit on mounts.\n",
> -			ve_name(get_exec_env()));
> +			ve_name(ve));
>   		return NULL;
>   	}
>   
> @@ -337,6 +340,14 @@ static struct mount *alloc_vfsmnt(const char *name, struct ve_struct *owner_ve)
>   	if (mnt) {
>   		int err;
>   
> +#ifdef CONFIG_VE
> +		/*
> +		 * Got ve reference in ve_try_reserve_mount, set mnt ve data
> +		 * here, so in case of error ve_mount_put sees correct info.
> +		 */
> +		mnt->ve_owner = ve;
> +#endif
> +
>   		err = mnt_alloc_id(mnt);
>   		if (err)
>   			goto out_free_cache;
> @@ -371,7 +382,8 @@ static struct mount *alloc_vfsmnt(const char *name, struct ve_struct *owner_ve)
>   		INIT_LIST_HEAD(&mnt->mnt_umounting);
>   		INIT_HLIST_HEAD(&mnt->mnt_stuck_children);
>   		mnt->mnt.mnt_idmap = &nop_mnt_idmap;
> -		ve_mount_nr_inc(mnt, owner_ve);
> +	} else {
> +		ve_mount_put(mnt, ve);
>   	}
>   	return mnt;

Sashiko says:
  In the function alloc_vfsmnt, can the decrement in ve_try_reserve_mount be
  invalid if the 'mnt_alloc_id' fails subsequently?
  This logic might lead to an incorrect mount count when the ve or the mount fails.

No, it can't. Passing ve_try_reserve_mount means mnt_avail was updated, so on
error path we must unconditionally increment it back.

>   
> @@ -382,6 +394,8 @@ static struct mount *alloc_vfsmnt(const char *name, struct ve_struct *owner_ve)
>   out_free_id:
>   	mnt_free_id(mnt);
>   out_free_cache:
> +	/* Got ve reference in ve_try_reserve_mount */

Sashiko says:
  The comment suggests that ve_mount_put sees a non-null mount in an error case,
  however, in test scenarios, isn't it possible for the ve to be incorrect?

No. ve is always correct, mnt = NULL is handled correctly by ve_mount_put.

> +	ve_mount_put(mnt, ve);
>   	kmem_cache_free(mnt_cache, mnt);
>   	return NULL;
>   }
> @@ -751,7 +765,9 @@ int sb_prepare_remount_readonly(struct super_block *sb)
>   static void free_vfsmnt(struct mount *mnt)
>   {
>   	mnt_idmap_put(mnt_idmap(&mnt->mnt));
> -	ve_mount_nr_dec(mnt);
> +#ifdef CONFIG_VE
> +	ve_mount_put(mnt, mnt->ve_owner);
> +#endif
>   	kfree_const(mnt->mnt_devname);
>   #ifdef CONFIG_SMP
>   	free_percpu(mnt->mnt_pcp);
> @@ -3214,7 +3230,7 @@ int ve_devmnt_process(struct ve_struct *ve, dev_t dev, void **data_pp, int remou
>   		if (devmnt->dev == dev) {
>   			err = ve_devmnt_check(data, devmnt->allowed_options);
>   			/*
> -			 * In case of @is_pseudouser set, ie restore procedure,
> +			 * In case of @is_pseudosuper set, ie restore procedure,
>   			 * we don't check for allowed options filtering, since
>   			 * restore mode is special.
>   			 */
> @@ -3353,30 +3369,30 @@ int ve_devmnt_verify(struct ve_struct *ve, dev_t dev, char *opts, bool new_mount
>   	return err;
>   }
>   
> -static inline int ve_mount_allowed(void)
> -{
> -	struct ve_struct *ve = get_exec_env();
> -
> -	return ve_is_super(ve) || ve->is_pseudosuper ||
> -		atomic_read(&ve->mnt_nr) < (int)sysctl_ve_mount_nr;
> -}
> -
> -static inline void ve_mount_nr_inc(struct mount *mnt, struct ve_struct *ve)
> +static inline int ve_try_reserve_mount(struct ve_struct *ve)
>   {
> -	if (!ve)
> -		ve = get_exec_env();
> +	int ret = ve_is_super(ve) || ve->is_pseudosuper;
> +	/* Ignore limits in ve0 and pseudosuper cases, but still count. */
> +	if (ret)
> +		atomic_dec(&ve->mnt_avail_nr);
> +	else
> +		ret = atomic_dec_if_positive(&ve->mnt_avail_nr) >= 0;

Sashiko says:
  Is there a chance that, in ve_try_reserve_mount, the return logic misleads the condition
  when ve is either ve0 or pseudosuper, thus bypassing necessary checks?
  This counting might allow unexpected mounts while avoiding constraints.

We want to ignore avail checks in ve0 and pseudosuper case.

>   
> -	mnt->ve_owner = get_ve(ve);
> -	atomic_inc(&ve->mnt_nr);
> +	if (ret)
> +		get_ve(ve);
> +	return ret;
>   }
>   
> -static inline void ve_mount_nr_dec(struct mount *mnt)
> +static inline void ve_mount_put(struct mount *mnt, struct ve_struct *ve)
>   {
> -	struct ve_struct *ve = mnt->ve_owner;
> -
> -	atomic_dec(&ve->mnt_nr);
> +	/*
> +	 * ve argument is needed to reuse this function in alloc_vfsmnt error path.
> +	 * Other users should pass mnt->ve_owner value.
> +	 */
> +	atomic_inc(&ve->mnt_avail_nr);

Sashio says:
  The unconditional increment in ve_mount_put may cause unintended changes in mnt_avail_nr
  when ve conditions are different. Could this result in inaccurate resource tracking?

No, ve_mount_put is called only when ve_try_reserve_mount is 1. It means it decremented
avail counter.

>   	put_ve(ve);
> -	mnt->ve_owner = NULL;
> +	if (mnt)
> +		mnt->ve_owner = NULL;
>   }
>   
>   ...

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel