* [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix @ 2019-07-29 15:13 Chuhong Yuan 2019-07-30 4:26 ` Kees Cook 0 siblings, 1 reply; 9+ messages in thread From: Chuhong Yuan @ 2019-07-29 15:13 UTC (permalink / raw) Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, linux-kernel, Chuhong Yuan strncmp(str, const, len) is error-prone. We had better use newly introduced str_has_prefix() instead of it. Signed-off-by: Chuhong Yuan <hslester96@gmail.com> --- kernel/cgroup/rdma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/cgroup/rdma.c b/kernel/cgroup/rdma.c index ae042c347c64..fd12a227f8e4 100644 --- a/kernel/cgroup/rdma.c +++ b/kernel/cgroup/rdma.c @@ -379,7 +379,7 @@ static int parse_resource(char *c, int *intval) return -EINVAL; return i; } - if (strncmp(value, RDMACG_MAX_STR, len) == 0) { + if (str_has_prefix(value, RDMACG_MAX_STR)) { *intval = S32_MAX; return i; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-07-29 15:13 [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix Chuhong Yuan @ 2019-07-30 4:26 ` Kees Cook 2019-07-30 6:39 ` Chuhong Yuan 0 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2019-07-30 4:26 UTC (permalink / raw) To: Chuhong Yuan Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, linux-kernel, Joe Perches, Laura Abbott, Michael Ellerman, Steven Rostedt On Mon, Jul 29, 2019 at 11:13:46PM +0800, Chuhong Yuan wrote: > strncmp(str, const, len) is error-prone. > We had better use newly introduced > str_has_prefix() instead of it. Wait, stop. :) After Laura called my attention to your conversion series, mpe pointed out that str_has_prefix() is almost redundant to strstarts() (from 2009), and the latter has many more users. Let's fix strstarts() match str_has_prefix()'s return behavior (all the existing callers are doing boolean tests, so the change in return value won't matter), and then we can continue with this replacement. (And add some documentation to Documenation/process/deprecated.rst along with a checkpatch.pl test maybe too?) Actually I'd focus first on the actually broken cases first (sizeof() without the "-1", etc): $ git grep strncmp.*sizeof | grep -v -- '-' | wc -l 17 I expect the "copy/paste" changes could just be a Coccinelle script that Linus could run to fix all the cases (and should be added to the kernel source's list of Coccinelle scripts). Especially since the bulk of the usage pattern are doing literals like this: arch/alpha/kernel/setup.c: if (strncmp(p, "mem=", 4) == 0) { $ git grep -E 'strncmp.*(sizeof|, *[0-9]*)' | wc -l 2565 And some cases are weirdly backwards: tools/perf/util/callchain.c: if (!strncmp(tok, "none", strlen(tok))) { -Kees > Signed-off-by: Chuhong Yuan <hslester96@gmail.com> > --- > kernel/cgroup/rdma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/cgroup/rdma.c b/kernel/cgroup/rdma.c > index ae042c347c64..fd12a227f8e4 100644 > --- a/kernel/cgroup/rdma.c > +++ b/kernel/cgroup/rdma.c > @@ -379,7 +379,7 @@ static int parse_resource(char *c, int *intval) > return -EINVAL; > return i; > } > - if (strncmp(value, RDMACG_MAX_STR, len) == 0) { > + if (str_has_prefix(value, RDMACG_MAX_STR)) { > *intval = S32_MAX; > return i; > } > -- > 2.20.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-07-30 4:26 ` Kees Cook @ 2019-07-30 6:39 ` Chuhong Yuan 2019-07-30 7:03 ` Joe Perches ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Chuhong Yuan @ 2019-07-30 6:39 UTC (permalink / raw) To: Kees Cook Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, linux-kernel, Joe Perches, Laura Abbott, Michael Ellerman, Steven Rostedt Kees Cook <keescook@chromium.org> 于2019年7月30日周二 下午12:26写道: > > On Mon, Jul 29, 2019 at 11:13:46PM +0800, Chuhong Yuan wrote: > > strncmp(str, const, len) is error-prone. > > We had better use newly introduced > > str_has_prefix() instead of it. > > Wait, stop. :) After Laura called my attention to your conversion series, > mpe pointed out that str_has_prefix() is almost redundant to strstarts() > (from 2009), and the latter has many more users. Let's fix strstarts() > match str_has_prefix()'s return behavior (all the existing callers are > doing boolean tests, so the change in return value won't matter), and > then we can continue with this replacement. (And add some documentation > to Documenation/process/deprecated.rst along with a checkpatch.pl test > maybe too?) > Thanks for your advice! Does that mean replacing strstarts()'s implementation with str_has_prefix()'s and then use strstarts() to substitute strncmp? I am not very clear about how to add the test into checkpatch.pl. Should I write a check for this pattern or directly add strncmp into deprecated_apis? > Actually I'd focus first on the actually broken cases first (sizeof() > without the "-1", etc): > > $ git grep strncmp.*sizeof | grep -v -- '-' | wc -l > 17 > > I expect the "copy/paste" changes could just be a Coccinelle script that > Linus could run to fix all the cases (and should be added to the kernel > source's list of Coccinelle scripts). Especially since the bulk of the > usage pattern are doing literals like this: > Actually I am using a Coccinelle script to detect the cases and have found 800+ places of strncmp(str, const, len). But the script still needs some improvement since it has false negatives and only focuses on detecting, not replacement. I can upload it after improvement. In which form should I upload it? In a patch's description or put it in coccinelle scripts? > arch/alpha/kernel/setup.c: if (strncmp(p, "mem=", 4) == 0) { > > $ git grep -E 'strncmp.*(sizeof|, *[0-9]*)' | wc -l > 2565 > > And some cases are weirdly backwards: > > tools/perf/util/callchain.c: if (!strncmp(tok, "none", strlen(tok))) { > > -Kees > I think with the help of Coccinelle script, all strncmp(str, const, len) can be replaced and these problems will be eliminated. :) Regards, Chuhong > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com> > > --- > > kernel/cgroup/rdma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/cgroup/rdma.c b/kernel/cgroup/rdma.c > > index ae042c347c64..fd12a227f8e4 100644 > > --- a/kernel/cgroup/rdma.c > > +++ b/kernel/cgroup/rdma.c > > @@ -379,7 +379,7 @@ static int parse_resource(char *c, int *intval) > > return -EINVAL; > > return i; > > } > > - if (strncmp(value, RDMACG_MAX_STR, len) == 0) { > > + if (str_has_prefix(value, RDMACG_MAX_STR)) { > > *intval = S32_MAX; > > return i; > > } > > -- > > 2.20.1 > > > > -- > Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-07-30 6:39 ` Chuhong Yuan @ 2019-07-30 7:03 ` Joe Perches 2019-07-30 15:01 ` Steven Rostedt 2019-07-30 13:47 ` Chuhong Yuan 2019-08-29 23:52 ` Kees Cook 2 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2019-07-30 7:03 UTC (permalink / raw) To: Chuhong Yuan, Kees Cook Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, linux-kernel, Laura Abbott, Michael Ellerman, Steven Rostedt On Tue, 2019-07-30 at 14:39 +0800, Chuhong Yuan wrote: > Kees Cook <keescook@chromium.org> 于2019年7月30日周二 下午12:26写道: > > On Mon, Jul 29, 2019 at 11:13:46PM +0800, Chuhong Yuan wrote: > > > strncmp(str, const, len) is error-prone. > > > We had better use newly introduced > > > str_has_prefix() instead of it. > > > > Wait, stop. :) After Laura called my attention to your conversion series, > > mpe pointed out that str_has_prefix() is almost redundant to strstarts() > > (from 2009), and the latter has many more users. Let's fix strstarts() > > match str_has_prefix()'s return behavior (all the existing callers are > > doing boolean tests, so the change in return value won't matter), and > > then we can continue with this replacement. (And add some documentation > > to Documenation/process/deprecated.rst along with a checkpatch.pl test > > maybe too?) > > > > Thanks for your advice! > Does that mean replacing strstarts()'s implementation with > str_has_prefix()'s and then use strstarts() to substitute > strncmp? > > I am not very clear about how to add the test into checkpatch.pl. > Should I write a check for this pattern or directly add strncmp into > deprecated_apis? After Nitin's patch gets applied: (which btw wasn't cc'd to lkml) (sorry about the bad link, something about it hits some spam filter) open wall . com/lists/kernel-hardening/2019/07/28/1 Add it to deprecated_string_apis And we've had this sort of discussion before: https://lore.kernel.org/patchwork/patch/1026598/ I believe I'm still in favor of global conversion of strstarts to str_has_prefix. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-07-30 7:03 ` Joe Perches @ 2019-07-30 15:01 ` Steven Rostedt 0 siblings, 0 replies; 9+ messages in thread From: Steven Rostedt @ 2019-07-30 15:01 UTC (permalink / raw) To: Joe Perches Cc: Chuhong Yuan, Kees Cook, Tejun Heo, Li Zefan, Johannes Weiner, cgroups, linux-kernel, Laura Abbott, Michael Ellerman On Tue, 30 Jul 2019 00:03:28 -0700 Joe Perches <joe@perches.com> wrote: > I believe I'm still in favor of global conversion of > strstarts to str_has_prefix. > So am I. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-07-30 6:39 ` Chuhong Yuan 2019-07-30 7:03 ` Joe Perches @ 2019-07-30 13:47 ` Chuhong Yuan 2019-08-02 12:16 ` Michael Ellerman 2019-08-29 23:52 ` Kees Cook 2 siblings, 1 reply; 9+ messages in thread From: Chuhong Yuan @ 2019-07-30 13:47 UTC (permalink / raw) To: Kees Cook Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, LKML, Joe Perches, Laura Abbott, Michael Ellerman, Steven Rostedt Chuhong Yuan <hslester96@gmail.com> 于2019年7月30日周二 下午2:39写道: > > Kees Cook <keescook@chromium.org> 于2019年7月30日周二 下午12:26写道: > > > > On Mon, Jul 29, 2019 at 11:13:46PM +0800, Chuhong Yuan wrote: > > > strncmp(str, const, len) is error-prone. > > > We had better use newly introduced > > > str_has_prefix() instead of it. > > > > Wait, stop. :) After Laura called my attention to your conversion series, > > mpe pointed out that str_has_prefix() is almost redundant to strstarts() > > (from 2009), and the latter has many more users. Let's fix strstarts() > > match str_has_prefix()'s return behavior (all the existing callers are > > doing boolean tests, so the change in return value won't matter), and > > then we can continue with this replacement. (And add some documentation > > to Documenation/process/deprecated.rst along with a checkpatch.pl test > > maybe too?) > > > > Thanks for your advice! > Does that mean replacing strstarts()'s implementation with > str_has_prefix()'s and then use strstarts() to substitute > strncmp? > > I am not very clear about how to add the test into checkpatch.pl. > Should I write a check for this pattern or directly add strncmp into > deprecated_apis? > > > Actually I'd focus first on the actually broken cases first (sizeof() > > without the "-1", etc): > > > > $ git grep strncmp.*sizeof | grep -v -- '-' | wc -l > > 17 > > > > I expect the "copy/paste" changes could just be a Coccinelle script that > > Linus could run to fix all the cases (and should be added to the kernel > > source's list of Coccinelle scripts). Especially since the bulk of the > > usage pattern are doing literals like this: > > > > Actually I am using a Coccinelle script to detect the cases and > have found 800+ places of strncmp(str, const, len). > But the script still needs some improvement since it has false > negatives and only focuses on detecting, not replacement. > I can upload it after improvement. > In which form should I upload it? In a patch's description or put it > in coccinelle scripts? > > > arch/alpha/kernel/setup.c: if (strncmp(p, "mem=", 4) == 0) { > > > > $ git grep -E 'strncmp.*(sizeof|, *[0-9]*)' | wc -l > > 2565 > > > > And some cases are weirdly backwards: > > > > tools/perf/util/callchain.c: if (!strncmp(tok, "none", strlen(tok))) { > > I find there are cases of this pattern are not wrong. One example is kernel/irq/debugfs.c: if (!strncmp(buf, "trigger", size)) { Thus I do not know whether I should include these cases in my script. > > -Kees > > > > I think with the help of Coccinelle script, all strncmp(str, const, len) > can be replaced and these problems will be eliminated. :) > > Regards, > Chuhong > > > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com> > > > --- > > > kernel/cgroup/rdma.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/cgroup/rdma.c b/kernel/cgroup/rdma.c > > > index ae042c347c64..fd12a227f8e4 100644 > > > --- a/kernel/cgroup/rdma.c > > > +++ b/kernel/cgroup/rdma.c > > > @@ -379,7 +379,7 @@ static int parse_resource(char *c, int *intval) > > > return -EINVAL; > > > return i; > > > } > > > - if (strncmp(value, RDMACG_MAX_STR, len) == 0) { > > > + if (str_has_prefix(value, RDMACG_MAX_STR)) { > > > *intval = S32_MAX; > > > return i; > > > } > > > -- > > > 2.20.1 > > > > > > > -- > > Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-07-30 13:47 ` Chuhong Yuan @ 2019-08-02 12:16 ` Michael Ellerman 0 siblings, 0 replies; 9+ messages in thread From: Michael Ellerman @ 2019-08-02 12:16 UTC (permalink / raw) To: Chuhong Yuan, Kees Cook Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, LKML, Joe Perches, Laura Abbott, Steven Rostedt Chuhong Yuan <hslester96@gmail.com> writes: > Chuhong Yuan <hslester96@gmail.com> 于2019年7月30日周二 下午2:39写道: >> Kees Cook <keescook@chromium.org> 于2019年7月30日周二 下午12:26写道: >> > On Mon, Jul 29, 2019 at 11:13:46PM +0800, Chuhong Yuan wrote: >> > > strncmp(str, const, len) is error-prone. >> > > We had better use newly introduced >> > > str_has_prefix() instead of it. >> > >> > Wait, stop. :) After Laura called my attention to your conversion series, >> > mpe pointed out that str_has_prefix() is almost redundant to strstarts() >> > (from 2009), and the latter has many more users. Let's fix strstarts() >> > match str_has_prefix()'s return behavior (all the existing callers are >> > doing boolean tests, so the change in return value won't matter), and >> > then we can continue with this replacement. (And add some documentation >> > to Documenation/process/deprecated.rst along with a checkpatch.pl test >> > maybe too?) >> > >> >> Thanks for your advice! >> Does that mean replacing strstarts()'s implementation with >> str_has_prefix()'s and then use strstarts() to substitute >> strncmp? >> >> I am not very clear about how to add the test into checkpatch.pl. >> Should I write a check for this pattern or directly add strncmp into >> deprecated_apis? >> >> > Actually I'd focus first on the actually broken cases first (sizeof() >> > without the "-1", etc): >> > >> > $ git grep strncmp.*sizeof | grep -v -- '-' | wc -l >> > 17 >> > >> > I expect the "copy/paste" changes could just be a Coccinelle script that >> > Linus could run to fix all the cases (and should be added to the kernel >> > source's list of Coccinelle scripts). Especially since the bulk of the >> > usage pattern are doing literals like this: >> > >> >> Actually I am using a Coccinelle script to detect the cases and >> have found 800+ places of strncmp(str, const, len). >> But the script still needs some improvement since it has false >> negatives and only focuses on detecting, not replacement. >> I can upload it after improvement. >> In which form should I upload it? In a patch's description or put it >> in coccinelle scripts? >> >> > arch/alpha/kernel/setup.c: if (strncmp(p, "mem=", 4) == 0) { >> > >> > $ git grep -E 'strncmp.*(sizeof|, *[0-9]*)' | wc -l >> > 2565 >> > >> > And some cases are weirdly backwards: >> > >> > tools/perf/util/callchain.c: if (!strncmp(tok, "none", strlen(tok))) { > > I find there are cases of this pattern are not wrong. > One example is kernel/irq/debugfs.c: if (!strncmp(buf, "trigger", size)) { > > Thus I do not know whether I should include these cases in my script. That case isn't looking for a prefix AFAICS, so you should skip it. I think Kees regexp was just slightly wrong, it should be: git grep -E 'strncmp.*(sizeof|, *[0-9]+)' ie. either literal "sizeof" or *at least one* digit. cheers ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-07-30 6:39 ` Chuhong Yuan 2019-07-30 7:03 ` Joe Perches 2019-07-30 13:47 ` Chuhong Yuan @ 2019-08-29 23:52 ` Kees Cook 2019-09-02 6:48 ` Chuhong Yuan 2 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2019-08-29 23:52 UTC (permalink / raw) To: Chuhong Yuan Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, linux-kernel, Joe Perches, Laura Abbott, Michael Ellerman, Steven Rostedt On Tue, Jul 30, 2019 at 02:39:40PM +0800, Chuhong Yuan wrote: > I think with the help of Coccinelle script, all strncmp(str, const, len) > can be replaced and these problems will be eliminated. :) Hi! Just pinging this thread again. Any progress on this conversion? Thanks! -- Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix 2019-08-29 23:52 ` Kees Cook @ 2019-09-02 6:48 ` Chuhong Yuan 0 siblings, 0 replies; 9+ messages in thread From: Chuhong Yuan @ 2019-09-02 6:48 UTC (permalink / raw) To: Kees Cook Cc: Tejun Heo, Li Zefan, Johannes Weiner, cgroups, LKML, Joe Perches, Laura Abbott, Michael Ellerman, Steven Rostedt On Fri, Aug 30, 2019 at 7:52 AM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jul 30, 2019 at 02:39:40PM +0800, Chuhong Yuan wrote: > > I think with the help of Coccinelle script, all strncmp(str, const, len) > > can be replaced and these problems will be eliminated. :) > > Hi! Just pinging this thread again. Any progress on this conversion? > I didn't work further on this conversion since it seems that developers are not very interested in this problem (only half of my sent patches have been responded till now) and I am working on other projects recently. If the Coccinelle script is needed, I can try to implement it next week. Regards, Chuhong > Thanks! > > -- > Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-09-02 6:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-29 15:13 [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix Chuhong Yuan 2019-07-30 4:26 ` Kees Cook 2019-07-30 6:39 ` Chuhong Yuan 2019-07-30 7:03 ` Joe Perches 2019-07-30 15:01 ` Steven Rostedt 2019-07-30 13:47 ` Chuhong Yuan 2019-08-02 12:16 ` Michael Ellerman 2019-08-29 23:52 ` Kees Cook 2019-09-02 6:48 ` Chuhong Yuan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).