[PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[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

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Kees Cook]

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

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Chuhong Yuan]

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

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Joe Perches]

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.

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Chuhong Yuan]

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

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Steven Rostedt]

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

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Michael Ellerman]

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

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Kees Cook]

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

Re: [PATCH 01/12] rdmacg: Replace strncmp with str_has_prefix

[Chuhong Yuan]

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