Date:   Sun, 19 Dec 2021 13:29:02 -0800
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Baokun Li <libaokun1@huawei.com>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     keescook@chromium.org, yzaikin@google.com,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        yukuai3@huawei.com, Hulk Robot <hulkci@huawei.com>
Subject: Re: [PATCH -next] sysctl: returns -EINVAL when a negative value is
 passed to proc_doulongvec_minmax
Message-ID: <Yb+kHuIFnCKcfM5l@bombadil.infradead.org>
References: <20211209085635.1288737-1-libaokun1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211209085635.1288737-1-libaokun1@huawei.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

On Thu, Dec 09, 2021 at 04:56:35PM +0800, Baokun Li wrote:
> When we pass a negative value to the proc_doulongvec_minmax() function,
> the function returns 0, but the corresponding interface value does not
> change.
> 
> we can easily reproduce this problem with the following commands:
>     `cd /proc/sys/fs/epoll`
>     `echo -1 > max_user_watches; echo $?; cat max_user_watches`
> 
> This function requires a non-negative number to be passed in, so when
> a negative number is passed in, -EINVAL is returned.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  kernel/sysctl.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 7f07b058b180..537d2f75faa0 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1149,10 +1149,9 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table,
>  					     sizeof(proc_wspace_sep), NULL);
>  			if (err)
>  				break;
> -			if (neg)
> -				continue;
> +
>  			val = convmul * val / convdiv;
> -			if ((min && val < *min) || (max && val > *max)) {
> +			if (neg || (min && val < *min) || (max && val > *max)) {
>  				err = -EINVAL;
>  				break;
>  			}

I'd much prefer if we stick to the pattern:

err = proc_get_long(...);
if (err || neg) {
	err = -EINVAL;
	break;
}        

Look at the other proc_get_long() uses.

But otherwise yes we should do this, please Cc Andrew Morton in your
next patch and I'll Ack it. Also extend the commit log to include that
proc_get_long() always returns -EINVAL on error and so we embrace the
pattern already used in other places where we also check for a negative
value and it is not allowed.

Did you get to inspect all other unsigned proc calls? If not feel free,
and thanks for your patch!!!

Curious do you have docs on Hulk Robot?

  Luis