lkml.org 
[lkml]   [2022]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3] sysctl: handle table->maxlen robustly for proc_dobool
    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
    >

    \
     
     \ /
      Last update: 2022-06-09 17:44    [W:4.447 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site