| Message ID | 20220522052624.21493-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] sysctl: handle table->maxlen properly for proc_dobool | expand |
On Sun, May 22, 2022 at 01:26:24PM +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. For > robustness, fix it by reimplementing proc_dobool() properly. > > 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> > --- Thanks for your patch Muchun! Does this fix an actualy issue? Because the comit log suggest so. If so is there a bug which is known or a reproducer which can be implemented to showcase that bug? The reason I ask is that we have automatic scrapers for bug fixes, and I tend to prefer to avoid giving those automatic scrapers the idea that a patch is a fix for a kernel bug when it it is not. If what you are change is an optimization then your commit log should clarify that. If you are fixing something then you must be clear about about the details I mentioned. And then, if it does fix an issue, how long has the issue been know, what are the consequences of it? And up to what kernel is this issue present for? Luis
On Sun, May 22, 2022 at 01:26:24PM +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. For > robustness, fix it by reimplementing proc_dobool() properly. > > 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> > --- > 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, > }, This hunk is fine -- it's a reasonable fix-up. > 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; > + unsigned int val = READ_ONCE(*data); > + int ret; > + > + /* Do not support arrays yet. */ > + if (table->maxlen != sizeof(bool)) > + return -EINVAL; > + > + tmp.maxlen = sizeof(val); > + tmp.data = &val; > + ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL); > + if (ret) > + return ret; > + if (write) > + WRITE_ONCE(*data, val ? true : false); > + return 0; > } This part I don't understand -- it just inlines do_proc_dobool_conv(), and I think detracts from readability.
On Tue, May 24, 2022 at 1:27 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Sun, May 22, 2022 at 01:26:24PM +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. For > > robustness, fix it by reimplementing proc_dobool() properly. > > > > 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> > > --- > > Thanks for your patch Muchun! > > Does this fix an actualy issue? Because the comit log suggest so. Thanks for taking a look. I think it is an improvement not a real bug fix. 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 counter-intuitive. So it is very easy to make mistakes. Should I add those into the commit log? Thanks. > If so is there a bug which is known or a reproducer which can be > implemented to showcase that bug? > > The reason I ask is that we have automatic scrapers for bug fixes, > and I tend to prefer to avoid giving those automatic scrapers > the idea that a patch is a fix for a kernel bug when it it is not. > If what you are change is an optimization then your commit log should > clarify that. > > If you are fixing something then you must be clear about about the > details I mentioned. And then, if it does fix an issue, how long > has the issue been know, what are the consequences of it? And up > to what kernel is this issue present for? > > Luis
On Tue, May 24, 2022 at 3:33 AM Kees Cook <keescook@chromium.org> wrote: > > On Sun, May 22, 2022 at 01:26:24PM +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. For > > robustness, fix it by reimplementing proc_dobool() properly. > > > > 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> > > --- > > 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, > > }, > > This hunk is fine -- it's a reasonable fix-up. > > > 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; > > + unsigned int val = READ_ONCE(*data); > > + int ret; > > + > > + /* Do not support arrays yet. */ > > + if (table->maxlen != sizeof(bool)) > > + return -EINVAL; > > + > > + tmp.maxlen = sizeof(val); > > + tmp.data = &val; > > + ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL); > > + if (ret) > > + return ret; > > + if (write) > > + WRITE_ONCE(*data, val ? true : false); > > + return 0; > > } > > This part I don't understand -- it just inlines do_proc_dobool_conv(), > and I think detracts from readability. > I think do_proc_dobool_conv() is an abuse of do_proc_dointvec() since do_proc_dointvec() expects a "int" type data instead of a "bool". As you can see, there is some cast from bool to int or int to bool in do_proc_dobool_conv(). And do_proc_dobool_conv() supports arrays, while proc_dobool() does not support. It is a little ugly to fix this in __do_proc_dointvec() (I have fixed it in v1 [1]). This version refers to proc_dou8vec_minmax(). For me, I think it is cleaner than v1, any thoughts? [1] https://lore.kernel.org/all/20220519125505.92400-1-songmuchun@bytedance.com/
On Tue, May 24, 2022 at 10:30:01AM +0800, Muchun Song wrote: > On Tue, May 24, 2022 at 1:27 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Sun, May 22, 2022 at 01:26:24PM +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. For > > > robustness, fix it by reimplementing proc_dobool() properly. > > > > > > 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> > > > --- > > > > Thanks for your patch Muchun! > > > > Does this fix an actualy issue? Because the comit log suggest so. > > Thanks for taking a look. > > I think it is an improvement not a real bug fix. Then please adjust the commit log accordingly. > 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 > counter-intuitive. So it is very easy to make mistakes. Should I add > those into the commit log? Yes that is useful information when doing patch review as well. Luis
On Wed, May 25, 2022 at 12:20 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Tue, May 24, 2022 at 10:30:01AM +0800, Muchun Song wrote: > > On Tue, May 24, 2022 at 1:27 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > On Sun, May 22, 2022 at 01:26:24PM +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. For > > > > robustness, fix it by reimplementing proc_dobool() properly. > > > > > > > > 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> > > > > --- > > > > > > Thanks for your patch Muchun! > > > > > > Does this fix an actualy issue? Because the comit log suggest so. > > > > Thanks for taking a look. > > > > I think it is an improvement not a real bug fix. > > Then please adjust the commit log accordingly. > > > 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 > > counter-intuitive. So it is very easy to make mistakes. Should I add > > those into the commit log? > > Yes that is useful information when doing patch review as well. > Thanks. I'll update in v3.
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, }, 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; + unsigned int val = READ_ONCE(*data); + int ret; + + /* Do not support arrays yet. */ + if (table->maxlen != sizeof(bool)) + return -EINVAL; + + tmp.maxlen = sizeof(val); + tmp.data = &val; + ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL); + if (ret) + return ret; + if (write) + WRITE_ONCE(*data, val ? true : false); + return 0; } /**
Setting ->proc_handler to proc_dobool at the same time setting ->maxlen to sizeof(int) is counter-intuitive, it is easy to make mistakes. For robustness, fix it by reimplementing proc_dobool() properly. 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> --- v2: - Reimplementing proc_dobool(). fs/lockd/svc.c | 2 +- kernel/sysctl.c | 38 +++++++++++++++++++------------------- 2 files changed, 20 insertions(+), 20 deletions(-)