Re: Re: Re: [PATCH] proc/sysctl: fix return error forproc_doulongvec_minmax

[Luis Chamberlain <mcgrof@kernel.org>]

On Wed, Dec 05, 2018 at 03:10:07PM +0800, cheng.lin130@zte.com.cn wrote:
> > On Mon, Dec 03, 2018 at 01:12:39PM +0800, cheng.lin130@zte.com.cn wrote:
> > > >Cheng, thanks for the patch!
> > > >
> > > >On Fri, Nov 30, 2018 at 02:35:17PM +0800, Cheng Lin wrote:
> > > >> If the number of input parameters is less than the total
> > > >> parameters, an INVAL error will be returned.
> > > >
> > > >Do you mean EINVAL?
> > > >
> > > Yes, it's EINVAL.
> > 
> > Please adjust the commit log.
> > 
> > > >> This patch ensure no error returned in this condition, just
> > > >> like other interfaces do.
> > > >
> > > >Have an actual example to reproduce?
> > > >
> > > >Luis
> > > >
> > > We use proc_doulongvec_minmax to pass up to two parameters with kern_table.
> > > e.g.
> >         {
> > >                 .procname       = "monitor_signals",
> > >                 .data           = &monitor_sigs,
> > >                 .maxlen         = 2*sizeof(unsigned long),
> > >                 .mode           = 0644,
> > >                 .proc_handler   = proc_doulongvec_minmax,
> > >         },
> > >
> > > Reproduce:
> > > When passing two parameters, it's work normal. But passing only one parameter, an error "Invalid argument"(EINVAL) is returned.
> > > [root@cl150 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals
> > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals
> > > 1       2
> > > [root@cl150 ~]# echo 3 > /proc/sys/kernel/monitor_signals
> > > -bash: echo: write error: Invalid argument
> > > [root@cl150 ~]# echo $?
> > > 1
> > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals
> > > 3       2
> > > [root@cl150 ~]#
> > >
> > > The following is the result after apply this patch. No error is returned when the number of input parameters is less than the total parameters.
> > > [root@cl150 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals
> > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals
> > > 1       2
> > > [root@cl150 ~]# echo 3 > /proc/sys/kernel/monitor_signals
> > > [root@cl150 ~]# echo $?
> > > 0
> > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals
> > > 3       2
> > > [root@cl150 ~]#
> > 
> > This would be good to have in the commit log as well. But your patch
> > only addresses one of the proc users, there are a few other checks like
> > this that would also need to be expanded for this. So please expand
> > your patch to cover the other cases as well.
> 
> I have done the check for the interfaces exported in kernel/sysctl.c.
> EXPORT_SYMBOL(proc_dointvec);
> EXPORT_SYMBOL(proc_douintvec);
> EXPORT_SYMBOL(proc_dointvec_jiffies);
> EXPORT_SYMBOL(proc_dointvec_minmax);
> EXPORT_SYMBOL_GPL(proc_douintvec_minmax);
> EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
> EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
> EXPORT_SYMBOL(proc_dostring);
> EXPORT_SYMBOL(proc_doulongvec_minmax);
> EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
> 
> The function call relationship is as follows. There are three processing functions dealing with digital parameters, __do_proc_dointvec/__do_proc_douintvec/__do_proc_doulongvec_minmax.
> 
> proc_dointvec------------------------------|
> proc_dointvec_jiffies----------------------|
> proc_dointvec_minmax------------------|
> proc_dointvec_userhz_jiffies------------|
> proc_dointvec_ms_jiffies-----------------|-> do_proc_dointvec----|-> __do_proc_dointvec
> 
> proc_douintvec-----------------------------|
> proc_douintvec_minmax-----------------|-> do_proc_douintvec---|->	__do_proc_douintvec
> 
> proc_doulongvec_minmax---------------|
> proc_doulongvec_ms_jiffies_minmax--|-> do_proc_doulongvec_minmax----|-> __do_proc_doulongvec_minmax

OK

> This patch deals with  __do_proc_doulongvec_minmax, just as
> __do_proc_dointvec does, adding a check for parameters 'left'. In
> __do_proc_douintvec, its code implementation explicitly does not
> support multiple inputs.  static int __do_proc_douintvec(...){
> 		...
>         /*
>          * Arrays are not supported, keep this simple. *Do not* add
>          * support for them.
>          */
>         if (vleft != 1) {
>                 *lenp = 0;
>                 return -EINVAL;
> 		...
> 		}
> 
> 		
> So, just __do_proc_doulongvec_minmax has the problem.  And most use of
> proc_doulongvec_minmax/proc_doulongvec_ms_jiffies_minmax just have one
> parameter.

The above text, up to my OK, is useful information for the commit log,
please add that.

> It's well hidden.

You mean that the issue is not widely spread? If so please add that
comment to the commit log, and resubmit a v2.

  Luis

> Typical multi-parameter applications for
> proc_dointvec, such as /proc/sys/kernel/printk.