Re: [Devel] [PATCH VZ10 v9 5/8] ve/ve.c: Generate VE resource accessors using macros

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

Sashiko review:

On 9/15/26 19:22, Vladimir Riabchun wrote:
> We have a bunch of almost identical code, which takes place
> and has some unclear variations in it. Unify all resource
> getters and setters using macros.
> 
> This changes behavior a little bit: some resources (netns number
> for example) required stopped VE. Now such limits could be updated
> with a running VE.
> Also, some max_nr_write functions didn't verify provided value,
> now it is not possible to pass overflowing value.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-135520
> 
> Feature: per-ve failcounters
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> ---
>   kernel/ve/ve.c | 209 +++++++++++++------------------------------------
>   1 file changed, 54 insertions(+), 155 deletions(-)
> 
> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> index a33e55991e52..5ebdb4b7baa7 100644
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c
> @@ -43,6 +43,39 @@
>   #include <linux/cpuset.h>		/* For css_tg() */
>   #include "../sched/sched.h"		/* For css_tg() */
>   
> +#define VE_RESOURCE(name)						\
> +static u64 ve_##name##_max_nr_read(struct cgroup_subsys_state *css,	\
> +				 struct cftype *cft)			\
> +{									\
> +	return css_to_ve(css)->name##_max_nr;				\
> +}									\
> +									\
> +static int ve_##name##_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);					\

Sashiko says:
  Does the change to use macros for VE resource management impact the safety
  of runtime VE modifications? The original ve_netns_max_nr_write function
  included a check on VE_IS_RUNNING(ve) || ve->ve_nsproxy, which prevented
  modifications unless these conditions were met.
  The new macro VE_RESOURCE seems to omit this check, which might lead to
  inconsistencies if resource limits are altered during VE operation.

max values are used only in these helpers to adjust avail number.
There is no reason for enforcing stopped VE - only host can update
the value anyway. And having the ability to adjust limits without stopping
CT seems really useful.

This is the anser for the next Sashiko comment:
  Is the current permission check sufficient in the VE_RESOURCE macro? By checking only
  ve_is_super(get_exec_env()), it seems you now allow modifications without verifying
  the VE's actual running state.
  Would this oversight instate authorizations in inappropriate scenarios where
  VE should disallow updates?
> +	delta = val - ve->name##_max_nr;				\

Sashiko says:
  Could the calculation of delta = val - ve->name_max_nr in VE_RESOURCE lead
  to integer overflow?

No, same reasoning as in previous patch.

> +	ve->name##_max_nr = val;					\
> +	atomic_add(delta, &ve->name##_avail_nr);			\
> +	up_write(&ve->op_sem);						\
> +	return 0;							\
> +}									\
> +									\
> +static s64 ve_##name##_avail_nr_read(struct cgroup_subsys_state *css,	\
> +				   struct cftype *cft)			\
> +{									\
> +	return atomic_read(&css_to_ve(css)->name##_avail_nr);		\
> +}
> +
>   extern struct kmapset_set sysfs_ve_perms_set;
>   #ifdef CONFIG_PROC_FS
>   extern struct kmapset_set proc_ve_perms_set;
> @@ -1036,121 +1069,10 @@ static int ve_features_write(struct cgroup_subsys_state *css, struct cftype *cft
>   	return 0;
>   }
>   
> -static u64 ve_netns_max_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
> -{
> -	return css_to_ve(css)->netns_max_nr;
> -}
> -
> -static int ve_netns_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;
> -
> -	down_write(&ve->op_sem);
> -	if (VE_IS_RUNNING(ve) || ve->ve_nsproxy) {
> -		up_write(&ve->op_sem);
> -		return -EBUSY;
> -	}
> -	delta = val - ve->netns_max_nr;
> -	ve->netns_max_nr = val;
> -	atomic_add(delta, &ve->netns_avail_nr);
> -	up_write(&ve->op_sem);
> -	return 0;
> -}
> -static u64 ve_netns_avail_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
> -{
> -	return atomic_read(&css_to_ve(css)->netns_avail_nr);
> -}
> -
> -static s64 ve_mnt_avail_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
> -{
> -	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);
> -	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;
> -}
> -
> -static int ve_netif_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->netif_max_nr;
> -	ve->netif_max_nr = val;
> -	atomic_add(delta, &ve->netif_avail_nr);
> -	up_write(&ve->op_sem);
> -	return 0;
> -}
> -
> -static s64 ve_netif_avail_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
> -{
> -	return atomic_read(&css_to_ve(css)->netif_avail_nr);
> -}
> -
> -static u64 ve_bpf_prog_max_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
> -{
> -	return css_to_ve(css)->bpf_prog_max_nr;
> -}
> -
> -static int ve_bpf_prog_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->bpf_prog_max_nr;
> -	ve->bpf_prog_max_nr = val;
> -	atomic_add(delta, &ve->bpf_prog_avail_nr);
> -	up_write(&ve->op_sem);
> -	return 0;
> -}
> -
> -static s64 ve_bpf_prog_avail_nr_read(struct cgroup_subsys_state *css, struct cftype *cft)
> -{
> -	return atomic_read(&css_to_ve(css)->bpf_prog_avail_nr);
> -}
> +VE_RESOURCE(netns);
> +VE_RESOURCE(mnt);
> +VE_RESOURCE(netif);
> +VE_RESOURCE(bpf_prog);
>   
>   static int ve_os_release_read(struct seq_file *sf, void *v)
>   {
> @@ -1598,6 +1520,19 @@ static int ve_rpc_kill_write(struct cgroup_subsys_state *css,
>   	return 0;
>   }
>   
> +/* checkpatch will report it, can't enclose this in parentheses. */
> +#define VE_RESOURCE_CFTYPE(res)					\
> +{								\
> +	.name			= #res "_max_nr",		\
> +	.flags			= CFTYPE_NOT_ON_ROOT,		\
> +	.read_u64		= ve_##res##_max_nr_read,	\
> +	.write_u64		= ve_##res##_max_nr_write,	\
> +},								\
> +{								\
> +	.name			= #res "_avail_nr",		\
> +	.read_s64		= ve_##res##_avail_nr_read,	\
> +}
> +
>   static struct cftype ve_cftypes[] = {
>   
>   	{
> @@ -1642,46 +1577,10 @@ static struct cftype ve_cftypes[] = {
>   		.read_u64		= ve_pid_max_read_u64,
>   		.write_u64		= ve_pid_max_write_running_u64,
>   	},
> -	{
> -		.name			= "netns_max_nr",
> -		.flags			= CFTYPE_NOT_ON_ROOT,
> -		.read_u64		= ve_netns_max_nr_read,
> -		.write_u64		= ve_netns_max_nr_write,
> -	},
> -	{
> -		.name			= "netns_avail_nr",
> -		.read_u64		= ve_netns_avail_nr_read,
> -	},
> -	{
> -		.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,
> -		.read_u64		= ve_netif_max_nr_read,
> -		.write_u64		= ve_netif_max_nr_write,
> -	},
> -	{
> -		.name			= "netif_avail_nr",
> -		.read_s64		= ve_netif_avail_nr_read,
> -	},
> -	{
> -		.name			= "bpf_prog_max_nr",
> -		.flags			= CFTYPE_NOT_ON_ROOT,
> -		.read_u64		= ve_bpf_prog_max_nr_read,
> -		.write_u64		= ve_bpf_prog_max_nr_write,
> -	},
> -	{
> -		.name			= "bpf_prog_avail_nr",
> -		.read_s64		= ve_bpf_prog_avail_nr_read,
> -	},
> +	VE_RESOURCE_CFTYPE(netns),
> +	VE_RESOURCE_CFTYPE(mnt),
> +	VE_RESOURCE_CFTYPE(netif),
> +	VE_RESOURCE_CFTYPE(bpf_prog),
>   	{
>   		.name			= "os_release",
>   		.max_write_len		= __NEW_UTS_LEN + 1,

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

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