linux-kernel - Re: [PATCH v2] mm/memcontrol: use kstrtobool for swapaccount param parsing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20220909145308.f2f61d6992f00ef6977f833b@linux-foundation.org>
Date:   Fri, 9 Sep 2022 14:53:08 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Liu Shixin <liushixin2@...wei.com>
Cc:     Jonathan Corbet <corbet@....net>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <songmuchun@...edance.com>,
        <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <cgroups@...r.kernel.org>, <linux-mm@...ck.org>,
        Kefeng Wang <wangkefeng.wang@...wei.com>
Subject: Re: [PATCH v2] mm/memcontrol: use kstrtobool for swapaccount param
 parsing

On Fri, 9 Sep 2022 16:46:47 +0800 Liu Shixin <liushixin2@...wei.com> wrote:

> Use kstrtobool which is more powerful to handle all kinds of parameters
> like 'Yy1Nn0' or [oO][NnFf] for "on" and "off".
> 
> ...
>
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6037,10 +6037,11 @@
>  			Execution Facility on pSeries.
>  
>  	swapaccount=	[KNL]
> -			Format: [0|1]
> +			Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 }
>  			Enable accounting of swap in memory resource
> -			controller if no parameter or 1 is given or disable
> -			it if 0 is given (See Documentation/admin-guide/cgroup-v1/memory.rst)
> +			controller if no parameter or [oO][Nn]/Y/y/1 is given
> +			or disable it if [oO][Ff]/N/n/0 is given
> +			(See Documentation/admin-guide/cgroup-v1/memory.rst)
>  
>  	swiotlb=	[ARM,IA-64,PPC,MIPS,X86]
>  			Format: { <int> [,<int>] | force | noforce }

mhocko suggested dropping this change as well.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 0a1a8a846870..5511c0c120d9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7434,10 +7434,10 @@ bool mem_cgroup_swap_full(struct folio *folio)
>  
>  static int __init setup_swap_account(char *s)
>  {
> -	if (!strcmp(s, "1"))
> -		cgroup_memory_noswap = false;
> -	else if (!strcmp(s, "0"))
> -		cgroup_memory_noswap = true;
> +	bool res;
> +
> +	if (!kstrtobool(s, &res))
> +		cgroup_memory_noswap = !res;
>  	return 1;
>  }
>  __setup("swapaccount=", setup_swap_account);

And I agree.  See, the risk with this patch is that someone will
develop userspace code which relies upon the new behaviour.  Then when
someone tries to use that code on an older kernel, whoops, it doesn't
work.  In other words, we're encouraging development of
non-backward-compatible userspace code.

Leaving the documentation as it was (just "0|1") will help to prevent
that situation.