Re: [Devel] [PATCH VZ10 v9 4/8] ve: Move from global VE mounts limit to per-VE limit

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

Sashiko review:

On 9/15/26 19:22, Vladimir Riabchun wrote:
> This change unifies mount limit handling with other VE limits.
> This will simplify refactoring later and allow more precise limits
> control from one place.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-135520
> 
> Feature: per-ve failcounters
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> ---
>   include/linux/ve.h                            |  9 +++--
>   kernel/ve/ve.c                                | 34 ++++++++++++++++++-
>   kernel/ve/veowner.c                           | 19 -----------
>   tools/testing/selftests/ve/ve_ns_owner_test.c |  6 +++-
>   4 files changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/ve.h b/include/linux/ve.h
> index 65c058c0f34d..fabc608e31f5 100644
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -96,7 +96,13 @@ struct ve_struct {
>   	atomic_t		nd_neigh_nr;
>   	unsigned long		meminfo_val;
>   
> +	/*
> +	 * Operations with a big amount of mount points can require a lot of time.
> +	 * These operations take the global lock namespace_sem, so they can affect
> +	 * other containers.
> +	 */
>   	atomic_t		mnt_avail_nr; /* number of available VE mounts */
> +	int			mnt_max_nr;
>   
>   #ifdef CONFIG_COREDUMP
>   	char			core_pattern[CORENAME_MAX_SIZE];
> @@ -143,8 +149,7 @@ extern int nr_ve;
>   #define NETNS_MAX_NR_DEFAULT	256	/* number of net-namespaces per-VE */
>   #define NETIF_MAX_NR_DEFAULT	256	/* number of net-interfaces per-VE */
>   #define BPF_PROG_MAX_NR_DEFAULT	256	/* number of loaded BPF progs per-VE */
> -
> -extern unsigned int sysctl_ve_mount_nr;
> +#define MNT_MAX_NR_DEFAULT	4096	/* number of mounts per-VE */
>   
>   #ifdef CONFIG_VE
>   static inline void ve_set_state(struct ve_struct *ve, int new_state)
> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> index 58a0c4b8e36e..a33e55991e52 100644
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c
> @@ -83,6 +83,7 @@ struct ve_struct ve0 = {
>   	.arp_neigh_nr		= ATOMIC_INIT(0),
>   	.nd_neigh_nr		= ATOMIC_INIT(0),
>   	.mnt_avail_nr		= ATOMIC_INIT(INT_MAX),
> +	.mnt_max_nr		= INT_MAX,
>   	.meminfo_val		= VE_MEMINFO_SYSTEM,
>   #ifdef CONFIG_X86
>   	.umh_running_helpers	= ATOMIC_INIT(0),
> @@ -787,7 +788,8 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
>   
>   	atomic_set(&ve->arp_neigh_nr, 0);
>   	atomic_set(&ve->nd_neigh_nr, 0);
> -	atomic_set(&ve->mnt_avail_nr, sysctl_ve_mount_nr);
> +	ve->mnt_max_nr = MNT_MAX_NR_DEFAULT;
> +	atomic_set(&ve->mnt_avail_nr, MNT_MAX_NR_DEFAULT);
>   
>   #ifdef CONFIG_COREDUMP
>   	strcpy(ve->core_pattern, "core");
> @@ -1068,6 +1070,30 @@ static s64 ve_mnt_avail_nr_read(struct cgroup_subsys_state *css, struct cftype *
>   	return atomic_read(&css_to_ve(css)->mnt_avail_nr);
>   }
>   
> +static u64 ve_mnt_max_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
> +{
> +	return css_to_ve(css)->mnt_max_nr;
> +}
> +
> +static int ve_mnt_max_nr_write(struct cgroup_subsys_state *css, struct cftype *cft, u64 val)
> +{
> +	struct ve_struct *ve = css_to_ve(css);
> +	int delta;
> +
> +	if (!ve_is_super(get_exec_env()))
> +		return -EPERM;
> +
> +	if (val > INT_MAX)
> +		return -EOVERFLOW;
> +
> +	down_write(&ve->op_sem);
> +	delta = val - ve->mnt_max_nr;
> +	ve->mnt_max_nr = val;
> +	atomic_add(delta, &ve->mnt_avail_nr);

Sashiko says:
  In the function ve_mnt_max_nr_write, the computation delta = val - ve->mnt_max_nr
  followed by atomic_add(delta, &ve->mnt_avail_nr) might lead to an integer
  overflow. What checks or safeguards are in place to ensure that this operation
  will not lead to an overflow, potentially affecting the VE system state?

0 <= val <= INT_MAX
0 <= ve->mnt_max_nr <= INT_MAX
INT_MIN < 0 - INT_MAX <= delta <= INT_MAX - 0

So delta never overflows. Can mnt_avail_nr overflow?
No, it can't. By definition mnt_avail_nr = mnt_max_nr - have_mounts.
It can't overflow INT_MAX since it's always <= mnt_max_nr <= INT_MAX.
It can't overflow INT_MIN - it means we have > |INT_MIN| mounts, which
is not the real case.

> +	up_write(&ve->op_sem);
> +	return 0;
> +}
> +
>   static u64 ve_netif_max_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
>   {
>   	return css_to_ve(css)->netif_max_nr;
> @@ -1630,6 +1656,12 @@ static struct cftype ve_cftypes[] = {
>   		.name			= "mnt_avail_nr",
>   		.read_s64		= ve_mnt_avail_nr_read,
>   	},
> +	{
> +		.name			= "mnt_max_nr",
> +		.flags			= CFTYPE_NOT_ON_ROOT,
> +		.read_u64		= ve_mnt_max_nr_read,
> +		.write_u64		= ve_mnt_max_nr_write,
> +	},
>   	{
>   		.name			= "netif_max_nr",
>   		.flags			= CFTYPE_NOT_ON_ROOT,
> diff --git a/kernel/ve/veowner.c b/kernel/ve/veowner.c
> index 1e631baf0de9..c77735f8c035 100644
> --- a/kernel/ve/veowner.c
> +++ b/kernel/ve/veowner.c
> @@ -48,26 +48,7 @@ static void prepare_proc(void)
>    * ------------------------------------------------------------------------
>    */
>   
> -/*
> - * Operations with a big amount of mount points can require a lot of time.
> - * These operations take the global lock namespace_sem, so they can affect
> - * other containers. Let us allow no more than sysctl_ve_mount_nr mount
> - * points for a VE.
> - */
> -unsigned int sysctl_ve_mount_nr = 4096;
> -static int ve_mount_nr_min = 0;
> -static int ve_mount_nr_max = INT_MAX;
> -
>   static struct ctl_table vz_fs_table[] = {
> -	{
> -		.procname	= "ve-mount-nr",
> -		.data		= &sysctl_ve_mount_nr,
> -		.maxlen		= sizeof(sysctl_ve_mount_nr),
> -		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> -		.extra1		= &ve_mount_nr_min,
> -		.extra2		= &ve_mount_nr_max,
> -	},

Sashiko says:
  The removal of sysctl_ve_mount_nr potentially affects users relying
  on this sysctl for VE mount limits, impacting backward compatibility.
  Is there an alternative provided for monitoring or configuring
  VE mount limits dynamically that maintains backward compatibility?

This sysctl mechanism doesn't work well with per-VE limits,
the only solution to maintain the behavior is to iterate over all
VEs and update avail numbers.

IMO this doesn't add that much value to have two separate limiting
mechanisms in different places.
>   	{
>   		.procname	= "fsync-enable",
>   		.data		= &ve0.fsync_enable,
> diff --git a/tools/testing/selftests/ve/ve_ns_owner_test.c b/tools/testing/selftests/ve/ve_ns_owner_test.c
> index c3fa7d7b7f9c..dcff4d54172d 100644
> --- a/tools/testing/selftests/ve/ve_ns_owner_test.c
> +++ b/tools/testing/selftests/ve/ve_ns_owner_test.c
> @@ -50,7 +50,7 @@
>    * any spurious accounting against the parent ve would overflow it.
>    */
>   #define VE_NETNS_MAX		3
> -#define VE_MOUNTS_MAX		4096
> +#define VE_MOUNTS_MAX		32
>   
>   /*
>    * Synchronisation across the clone() boundary: child does its setup,
> @@ -173,6 +173,10 @@ FIXTURE_SETUP(ve_ns_owner)
>   	snprintf(val, sizeof(val), "%d", VE_NETNS_MAX);
>   	ASSERT_EQ(write_file_at(self->cgv2_fd, path, val), 0);
>   
> +	snprintf(path, sizeof(path), "%d/ve.mnt_max_nr", self->ctid);
> +	snprintf(val, sizeof(val), "%d", VE_MOUNTS_MAX);
> +	ASSERT_EQ(write_file_at(self->cgv2_fd, path, val), 0);
> +
>   	/*
>   	 * The new ve cgroup has not been entered by anything yet, so its
>   	 * mnt_avail_nr counter should be VE_MOUNTS_MAX. Each test below

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

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