On Thu, Jun 9, 2022 at 11:42 PM Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote:
>
> Please Cc the original authors of code if sending some follow up
> possible enhancements.
Will do. +Jia He
>
> 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.
I am writing a code like:
static bool optimize_vmemmap_enabled;
static struct ctl_table hugetlb_vmemmap_sysctls[] = {
{
.procname = "hugetlb_optimize_vmemmap",
.data = &optimize_vmemmap_enabled,
.maxlen = sizeof(optimize_vmemmap_enabled),
.mode = 0644,
.proc_handler = proc_dobool,
},
{ }
};
At least I don't see any warnings from compiler. And I found
the assignment of ".data" should be "sizeof(int)", otherwise,
it does not work properly. It is a little weird to me.
>
> > 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.
Clearer! Thanks.
>
> > So it is an improvement not a real bug
> > fix.
> >
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Iurii Zaikin <yzaikin@xxxxxxxxxx>
> > ---
> > 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 afraid not. Since this change of proc_dobool will break the
"nsm_use_hostnames". It should be changed to
sizeof(nsm_use_hostnames) at the same time.
> I see it is only used once so the one commit should mention that also.
Well, will do.
>
> 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.
Make sense. I'll mention those things into 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.
At least now it cannot be NULL (no users do this now).
>
> And, so if it was false but not NULL, would it never do anything?
I think we can add the check of NULL in the future if it could be
happened, just like proc_dou8vec_minmax and proc_do_static_key
do (they do not check ->data as well).
>
> You can use lib/test_sysctl.c for this to proove / disprove correct
> functionality.
I didn't see the test for proc_bool in lib/test_sysctl.c. I think we can
add a separate patch later to add a test for proc_bool.
>
> > + 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.
If you agree reusing do_proc_douintvec to implement proc_dobool(),
I think a separate commit may be not suitable since do_proc_douintvec()
only support non-array. Mentioning this in commit log makes sense to me.
>
> > +
> > + 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?
Since we reuse do_proc_douintvec(), which requires a uint type, to
get/set the value from/to the users. I think you can refer to the implementation
of proc_dou8vec_minmax().
>
> > + 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 implementation of __do_proc_douintvec() is already only deal
with non-array. Maybe it is better to reuse __do_proc_douintvec()? Otherwise,
we need to implement a similar functionality (do_proc_do_bool) like it but just
process bool type. I suspect the changes will be not small. I am wondering is it
value to do this? If yes, should we also rework proc_dou8vec_minmax() as well?
Thanks.
>
> 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
> >
