Re: [RFC] kernel/sysctl.c: return -EINVAL when write invalid val to ulong type sysctl

From: subashab@codeaurora.org
To: Yisheng Xie <xieyisheng1@huawei.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	Mel Gorman <mgorman@techsingularity.net>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Johannes Weiner <hannes@cmpxchg.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Daniel Cashman <dcashman@google.com>, Willy Tarreau <w@1wt.eu>,
	Arnd Bergmann <arnd@arndb.de>, Hanjun Guo <guohanjun@huawei.com>,
	Xishi Qiu <qiuxishi@huawei.com>
Subject: Re: [RFC] kernel/sysctl.c: return -EINVAL when write invalid val to ulong type sysctl
Date: Sat, 26 Nov 2016 16:15:38 -0700	[thread overview]
Message-ID: <6d58a831975bfebea981289c6a1fd4bc@codeaurora.org> (raw)
In-Reply-To: <669ac5a2-8b2f-5a47-305d-f6cf7cff4f63@huawei.com>

On 2016-11-26 02:13, Yisheng Xie wrote:
> I tried to echo an invalid value to an unsigned long type sysctl on 
> 4.9.0-rc6:
>    linux:~# cat /proc/sys/vm/user_reserve_kbytes
>    131072
>    linux:~# echo -1 > /proc/sys/vm/user_reserve_kbytes
>    linux:~# cat /proc/sys/vm/user_reserve_kbytes
>    131072
> 
> The echo operation got error and the value do not write to 
> user_reserve_kbytes,
> however, user do not know it until check the value again.
> 
> Is it more suitable to return -EINVAL when echo an invalid value to an
> unsigned long
> type sysctl, in order to let user know what happened without checking
> its value once more?
> Just as what int type sysctl do:
>    linux:~#cat /proc/sys/kernel/sysctl_writes_strict
>    1
>    linux:~# echo 3 > /proc/sys/kernel/sysctl_writes_strict
>    bash: echo: write error: Invalid argument
> 
> ----------------------
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 706309f..40e9285 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2485,10 +2485,14 @@ static int __do_proc_doulongvec_minmax(void
> *data, struct ctl_table *table, int
>                                              sizeof(proc_wspace_sep), 
> NULL);
>                         if (err)
>                                 break;
> -                       if (neg)
> -                               continue;
> -                       if ((min && val < *min) || (max && val > *max))
> -                               continue;
> +                       if (neg) {
> +                               err = -EINVAL;
> +                               break;
> +                       }
> +                       if ((min && val < *min) || (max && val > *max)) 
> {
> +                               err = -EINVAL;
> +                               break;
> +                       }
>                         *i = val;
>                 } else {
>                         val = convdiv * (*i) / convmul;

Agree, this should be similar to proc_douintvec

root@vm:~# echo 8192 > /proc/sys/net/core/xfrm_aevent_rseqth
root@vm:~# cat /proc/sys/net/core/xfrm_aevent_rseqth
8192
root@vm:~# echo -1 > /proc/sys/net/core/xfrm_aevent_rseqth
-bash: echo: write error: Invalid argument
root@vm:~# cat /proc/sys/net/core/xfrm_aevent_rseqth
8192

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project