On Wed, Dec 05, 2018 at 03:10:07PM +0800, cheng.lin130@xxxxxxxxxx wrote:
> > On Mon, Dec 03, 2018 at 01:12:39PM +0800, cheng.lin130@xxxxxxxxxx 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.
