Re: [PATCH] mm/vmstat: reject invalid stat_interval values

[Michal Hocko <mhocko@suse.com>]

On Fri 10-04-26 19:25:54, Cao Ruichuang wrote:
> vm.stat_interval is exposed in seconds, but proc_dointvec_jiffies()
> currently accepts zero and negative values. In the current tree,
> writing 0 succeeds and leaves /proc/sys/vm/stat_interval at 0.
> 
> vmstat_update() uses the stored jiffy interval directly when it
> requeues its delayed work, so a zero interval can drive excessive
> kworker CPU usage. Negative values are not meaningful either.
> 
> Switch vm.stat_interval to a small custom sysctl handler that keeps
> the existing seconds-based userspace ABI while rejecting values below
> 1 and capping the upper bound at INT_MAX / HZ.

This is admin only interface and we usually trust them to know what they
are doing. The same applies here. There are many interface behaving this
way. Is there any specific reason why we should treat this one
differently or are you suggesting to change this approach and sanitize
all of them? Changing just this one in isolation is IMHO not worth it.

> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=220226
> Signed-off-by: Cao Ruichuang <create0818@163.com>
> ---
>  mm/vmstat.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 86b14b0f77b..f48d3bdad64 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1964,6 +1964,7 @@ static const struct seq_operations vmstat_op = {
>  #ifdef CONFIG_SMP
>  static DEFINE_PER_CPU(struct delayed_work, vmstat_work);
>  static int sysctl_stat_interval __read_mostly = HZ;
> +static const int sysctl_stat_interval_max = INT_MAX / HZ;
>  static int vmstat_late_init_done;
>  
>  #ifdef CONFIG_PROC_FS
> @@ -1972,6 +1973,26 @@ static void refresh_vm_stats(struct work_struct *work)
>  	refresh_cpu_vm_stats(true);
>  }
>  
> +static int vmstat_stat_interval_handler(const struct ctl_table *table, int write,
> +					void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	int interval = sysctl_stat_interval / HZ;
> +	const struct ctl_table tmp = {
> +		.data	= &interval,
> +		.maxlen	= sizeof(interval),
> +		.mode	= table->mode,
> +		.extra1	= SYSCTL_ONE,
> +		.extra2	= (void *)&sysctl_stat_interval_max,
> +	};
> +	int ret;
> +
> +	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> +	if (!ret && write)
> +		sysctl_stat_interval = interval * HZ;
> +
> +	return ret;
> +}
> +
>  static int vmstat_refresh(const struct ctl_table *table, int write,
>  		   void *buffer, size_t *lenp, loff_t *ppos)
>  {
> @@ -2236,7 +2257,7 @@ static const struct ctl_table vmstat_table[] = {
>  		.data		= &sysctl_stat_interval,
>  		.maxlen		= sizeof(sysctl_stat_interval),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_jiffies,
> +		.proc_handler	= vmstat_stat_interval_handler,
>  	},
>  	{
>  		.procname	= "stat_refresh",
> -- 
> 2.39.5 (Apple Git-154)

-- 
Michal Hocko
SUSE Labs