Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 9 Jun 2022 08:42:21 -0700
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Muchun Song <songmuchun@bytedance.com>,
        Kees Cook <keescook@chromium.org>, Jia He <hejianet@gmail.com>,
        Pan Xinhui <xinhui@linux.vnet.ibm.com>,
        Thomas Huth <thuth@redhat.com>,
        Chuck Lever <chuck.lever@oracle.com>
Cc:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        willy@infradead.org, duanxiongchun@bytedance.com,
        Iurii Zaikin <yzaikin@google.com>
Subject: Re: [PATCH v3] sysctl: handle table->maxlen robustly for proc_dobool
Message-ID: <YqIU3U+l1EDy7OgZ@bombadil.infradead.org>
References: <20220525065050.38905-1-songmuchun@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220525065050.38905-1-songmuchun@bytedance.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

Please Cc the original authors of code if sending some follow up
possible enhancements.

On Wed, May 25, 2022 at 02:50:50PM +0800, Muchun Song wrote:
> Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> to sizeof(int) is counter-intuitive, it is easy to make mistakes in the
> future (When I first use proc_dobool() in my driver, I assign
> sizeof(variable) to table->maxlen.  Then I found it was wrong, it should
> be sizeof(int) which was very counter-intuitive). 

How did you find this out? If I change fs/lockd/svc.c's use I get
compile warnings on at least x86_64.

> For robustness,
> rework proc_dobool() robustly. 

You mention robustness twice. Just say something like:

To help make things clear, make the logic used by proc_dobool() very
clear with regards to its requirement with working with bools.

> So it is an improvement not a real bug
> fix.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Iurii Zaikin <yzaikin@google.com>
> ---
> v3:
>  - Update commit log.
> 
> v2:
>  - Reimplementing proc_dobool().
> 
>  fs/lockd/svc.c  |  2 +-
>  kernel/sysctl.c | 38 +++++++++++++++++++-------------------
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 59ef8a1f843f..6e48ee787f49 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -496,7 +496,7 @@ static struct ctl_table nlm_sysctls[] = {
>  	{
>  		.procname	= "nsm_use_hostnames",
>  		.data		= &nsm_use_hostnames,
> -		.maxlen		= sizeof(int),
> +		.maxlen		= sizeof(nsm_use_hostnames),
>  		.mode		= 0644,
>  		.proc_handler	= proc_dobool,
>  	},

Should this be a separate patch? What about the rest of the kernel?
I see it is only used once so the one commit should mention that also.

Or did chaning this as you have it now alter the way the kernel
treats this sysctl? All these things would be useful to clarify
in the commit log.

> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index e52b6e372c60..50a2c29efc94 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -423,21 +423,6 @@ static void proc_put_char(void **buf, size_t *size, char c)
>  	}
>  }
>  
> -static int do_proc_dobool_conv(bool *negp, unsigned long *lvalp,
> -				int *valp,
> -				int write, void *data)
> -{
> -	if (write) {
> -		*(bool *)valp = *lvalp;
> -	} else {
> -		int val = *(bool *)valp;
> -
> -		*lvalp = (unsigned long)val;
> -		*negp = false;
> -	}
> -	return 0;
> -}
> -
>  static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
>  				 int *valp,
>  				 int write, void *data)
> @@ -708,16 +693,31 @@ int do_proc_douintvec(struct ctl_table *table, int write,
>   * @lenp: the size of the user buffer
>   * @ppos: file position
>   *
> - * Reads/writes up to table->maxlen/sizeof(unsigned int) integer
> - * values from/to the user buffer, treated as an ASCII string.
> + * Reads/writes up to table->maxlen/sizeof(bool) bool values from/to
> + * the user buffer, treated as an ASCII string.
>   *
>   * Returns 0 on success.
>   */
>  int proc_dobool(struct ctl_table *table, int write, void *buffer,
>  		size_t *lenp, loff_t *ppos)
>  {
> -	return do_proc_dointvec(table, write, buffer, lenp, ppos,
> -				do_proc_dobool_conv, NULL);
> +	struct ctl_table tmp = *table;
> +	bool *data = table->data;

Previously do_proc_douintvec() is called, and that checks if table->data
is NULL previously before reading it and if so bails on
__do_proc_dointvec() as follows:

        if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
		*lenp = 0;
		return 0;
	}

Is it possible to have table->data be NULL? I think that's where the
above check comes from.

And, so if it was false but not NULL, would it never do anything?

You can use lib/test_sysctl.c for this to proove / disprove correct
functionality.

> +	unsigned int val = READ_ONCE(*data);
> +	int ret;
> +
> +	/* Do not support arrays yet. */
> +	if (table->maxlen != sizeof(bool))
> +		return -EINVAL;

This is a separate change, and while I agree with it, as it simplifies
our implementation and we don't want to add more array crap support,
this should *alone* should be a separate commit.

> +
> +	tmp.maxlen = sizeof(val);

Why even set this as you do when we know it must be sizeof(bool)?
Or would this break things given do_proc_douintvec() is used?

> +	tmp.data = &val;
> +	ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL);

Ugh, since we are avoiding arrays and we are only dealing with bools
I'm inclined to just ask we simpify this a bool implementation which
does something like do_proc_do_bool() but without array and is optimized
just for bools.

The current hoops to read this code is simplly irritating

  Luis

> +	if (ret)
> +		return ret;
> +	if (write)
> +		WRITE_ONCE(*data, val ? true : false);
> +	return 0;
>  }
>  
>  /**
> -- 
> 2.11.0
>