Re: [PATCH 1/1] mm/page_alloc: enforce upper bound on min_free_kbytes sysctl write

[Michal Hocko]

On Fri 18-09-26 11:45:51, Mohammed EL Kadiri wrote:
> syzbot reports a kernel panic ("System is deadlocked on memory")
> after writing a large value (e.g. 3145728, 3GB) to
> /proc/sys/vm/min_free_kbytes.
> 
> calculate_min_free_kbytes() already caps the auto-tuned default at
> 256MB, with a comment noting larger values don't make sense even on
> big machines. But that cap only applies to the computed default --
> min_free_kbytes_sysctl_handler() lets a user write any value, since
> it only sets extra1 = SYSCTL_ZERO, with no extra2.
> 
> An unbounded min_free_kbytes drives watermarks past what the system
> can provide, so every allocation triggers reclaim/OOM with no way to
> make progress, and out_of_memory() eventually panics.

This is an admin only interface and we assume whoever has access can
realize consequences of the set value. Auto-tuning really cannot make
any judgment call so it needs a cap. The same doens't hold for the
admin.

> Fix this by applying the same 256MB cap to the sysctl write path.
> 
> Tested on top of upstream master (5dd1818b15d9):
>   - Before: syzbot's reproducer reliably panics the kernel.
>   - After: writing 3145728 fails with -EINVAL, normal values
>     (e.g. 200000) still work, and the reproducer no longer panics
>     the kernel across repeated runs.
> 
> Note: this covers the reported case (a value far above any real
> machine's RAM). It doesn't prevent a smaller value that's still a
> large fraction of a very small system's memory from causing the
> same problem via lowmem_pages in __setup_per_zone_wmarks(). Open to
> a follow-up there if maintainers think it's worth it.
> 
> Reported-by: syzbot+c61d6962d0b7e698439e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=c61d6962d0b7e698439e
> Signed-off-by: Mohammed EL Kadiri <med08elkadiri@xxxxxxxxx>

There is nothing really to fix.
Nacked-by: Michal Hocko <mhocko@xxxxxxxx>

> ---
>  mm/page_alloc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 12fac9084c48..8cb62879bf41 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6933,6 +6933,9 @@ static int percpu_pagelist_high_fraction_sysctl_handler(const struct ctl_table *
>  	return ret;
>  }
>  
> +/* Cap sysctl writes to the same bound calculate_min_free_kbytes() uses. */
> +static const int max_user_min_free_kbytes = 262144;
> +
>  static const struct ctl_table page_alloc_sysctl_table[] = {
>  	{
>  		.procname	= "min_free_kbytes",
> @@ -6941,6 +6944,7 @@ static const struct ctl_table page_alloc_sysctl_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= min_free_kbytes_sysctl_handler,
>  		.extra1		= SYSCTL_ZERO,
> +		.extra2		= (void *)&max_user_min_free_kbytes,
>  	},
>  	{
>  		.procname	= "watermark_boost_factor",
> -- 
> 2.53.0

-- 
Michal Hocko
SUSE Labs