[PATCH] arm64/mm: Reject invalid NUMA option

[PATCH] arm64/mm: Reject invalid NUMA option

[Gavin Shan]

The NUMA option is parsed by str_has_prefix() and the invalid option
like "numa=o" can be regarded as "numa=off" wrongly.

This fixes the issue with sysfs_streq(), which have more sanity checks,
to avoid accepting the invalid options.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/mm/numa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index 4decf1659700..bd458b28616a 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c
@@ -29,7 +29,8 @@ static __init int numa_parse_early_param(char *opt)
 {
 	if (!opt)
 		return -EINVAL;
-	if (str_has_prefix(opt, "off"))
+
+	if (sysfs_streq(opt, "off"))
 		numa_off = true;
 
 	return 0;
-- 
2.23.0

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Mark Rutland]

[Adding Steve, who added str_has_prefix()]

On Fri, Apr 24, 2020 at 02:53:14PM +1000, Gavin Shan wrote:
> The NUMA option is parsed by str_has_prefix() and the invalid option
> like "numa=o" can be regarded as "numa=off" wrongly.

Are you certain that can pass? If that can happen, str_has_prefix() is
misnamed and does not seem to do what its kerneldoc says it does, as
"off" is not a prefix of "o".

> This fixes the issue with sysfs_streq(), which have more sanity checks,
> to avoid accepting the invalid options.

That doesn't sound immediately right, since this is an early parameter,
which has nothing to do with sysfs. Perhaps that's just a misleading
name?

Thanks,
Mark.

> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/mm/numa.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index 4decf1659700..bd458b28616a 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -29,7 +29,8 @@ static __init int numa_parse_early_param(char *opt)
>  {
>  	if (!opt)
>  		return -EINVAL;
> -	if (str_has_prefix(opt, "off"))
> +
> +	if (sysfs_streq(opt, "off"))
>  		numa_off = true;
>  
>  	return 0;
> -- 
> 2.23.0
> 

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Gavin Shan]

Hi Mark,

On 4/24/20 8:11 PM, Mark Rutland wrote:
> [Adding Steve, who added str_has_prefix()]
> 
> On Fri, Apr 24, 2020 at 02:53:14PM +1000, Gavin Shan wrote:
>> The NUMA option is parsed by str_has_prefix() and the invalid option
>> like "numa=o" can be regarded as "numa=off" wrongly.
> 
> Are you certain that can pass? If that can happen, str_has_prefix() is
> misnamed and does not seem to do what its kerneldoc says it does, as
> "off" is not a prefix of "o".
> 

Yes, It's possible. str_has_prefix() depends on strncmp(). In this particular
case, it's equal to the snippet of code as below: strncmp() returns zero.
str_has_prefix() returns 3.

int strncmp(const char *cs, const char *ct, size_t count)
{
         unsigned char c1, c2;

         while (count) {
                 c1 = *cs++;
                 c2 = *ct++;
                 if (c1 != c2)
                         return c1 < c2 ? -1 : 1;
                 if (!c1)                             /* break after first character is compared */
                         break;
                 count--;
         }
         return 0;                                    /* 0 returned */
}

static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
{
         size_t len = strlen("o");
         return strncmp("o", "off", 1) == 0 ? len : 0;
}

>> This fixes the issue with sysfs_streq(), which have more sanity checks,
>> to avoid accepting the invalid options.
> 
> That doesn't sound immediately right, since this is an early parameter,
> which has nothing to do with sysfs. Perhaps that's just a misleading
> name?
> 

sysfs_streq() was introduced to compare the parameters received from sysfs
entry, but I don't think it has to be necessarily tied with sysfs entry.
So the name is bit misleading. Alternatively, we also can fix it in another
way (as below) if we try to avoid using sysfs_streq().

diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index 4decf1659700..b0c1ec78f50f 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c
@@ -29,9 +29,13 @@ static __init int numa_parse_early_param(char *opt)
  {
         if (!opt)
                 return -EINVAL;
-       if (str_has_prefix(opt, "off"))
+
+       if (strlen(opt) >= 3 && str_has_prefix(opt, "off"))
                 numa_off = true;

> Thanks,
> Mark.
> 

Thanks,
Gavin

>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   arch/arm64/mm/numa.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index 4decf1659700..bd458b28616a 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -29,7 +29,8 @@ static __init int numa_parse_early_param(char *opt)
>>   {
>>   	if (!opt)
>>   		return -EINVAL;
>> -	if (str_has_prefix(opt, "off"))
>> +
>> +	if (sysfs_streq(opt, "off"))
>>   		numa_off = true;
>>   
>>   	return 0;
>> -- 
>> 2.23.0
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Steven Rostedt]

On Tue, 28 Apr 2020 10:59:14 +1000
Gavin Shan <gshan@redhat.com> wrote:

> Hi Mark,
> 
> On 4/24/20 8:11 PM, Mark Rutland wrote:
> > [Adding Steve, who added str_has_prefix()]
> > 
> > On Fri, Apr 24, 2020 at 02:53:14PM +1000, Gavin Shan wrote:  
> >> The NUMA option is parsed by str_has_prefix() and the invalid option
> >> like "numa=o" can be regarded as "numa=off" wrongly.  
> > 
> > Are you certain that can pass? If that can happen, str_has_prefix() is
> > misnamed and does not seem to do what its kerneldoc says it does, as
> > "off" is not a prefix of "o".
> >   
> 
> Yes, It's possible. str_has_prefix() depends on strncmp(). In this particular
> case, it's equal to the snippet of code as below: strncmp() returns zero.
> str_has_prefix() returns 3.

Wait! strncmp("o", "off", 3) returns zero?

That to me looks like a bug!

This means str_has_prefix() is broken in other areas as well.


> 
> int strncmp(const char *cs, const char *ct, size_t count)
> {
>          unsigned char c1, c2;
> 
>          while (count) {
>                  c1 = *cs++;
>                  c2 = *ct++;
>                  if (c1 != c2)
>                          return c1 < c2 ? -1 : 1;
>                  if (!c1)                             /* break after first character is compared */

Crap! That is totally wrong!

/me goes to fix...

-- Steve

>                          break;
>                  count--;
>          }
>          return 0;                                    /* 0 returned */
> }
> 
> static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> {
>          size_t len = strlen("o");
>          return strncmp("o", "off", 1) == 0 ? len : 0;
> }
> 
> >> This fixes the issue with sysfs_streq(), which have more sanity checks,
> >> to avoid accepting the invalid options.  
> > 
> > That doesn't sound immediately right, since this is an early parameter,
> > which has nothing to do with sysfs. Perhaps that's just a misleading
> > name?
> >   
> 
> sysfs_streq() was introduced to compare the parameters received from sysfs
> entry, but I don't think it has to be necessarily tied with sysfs entry.
> So the name is bit misleading. Alternatively, we also can fix it in another
> way (as below) if we try to avoid using sysfs_streq().
> 
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index 4decf1659700..b0c1ec78f50f 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -29,9 +29,13 @@ static __init int numa_parse_early_param(char *opt)
>   {
>          if (!opt)
>                  return -EINVAL;
> -       if (str_has_prefix(opt, "off"))
> +
> +       if (strlen(opt) >= 3 && str_has_prefix(opt, "off"))
>                  numa_off = true;
> 
> > Thanks,
> > Mark.
> >   
> 
> Thanks,
> Gavin
> 
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >>   arch/arm64/mm/numa.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> >> index 4decf1659700..bd458b28616a 100644
> >> --- a/arch/arm64/mm/numa.c
> >> +++ b/arch/arm64/mm/numa.c
> >> @@ -29,7 +29,8 @@ static __init int numa_parse_early_param(char *opt)
> >>   {
> >>   	if (!opt)
> >>   		return -EINVAL;
> >> -	if (str_has_prefix(opt, "off"))
> >> +
> >> +	if (sysfs_streq(opt, "off"))
> >>   		numa_off = true;
> >>   
> >>   	return 0;
> >> -- 
> >> 2.23.0
> >>  
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >   

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Steven Rostedt]

On Mon, 27 Apr 2020 22:54:06 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 28 Apr 2020 10:59:14 +1000
> Gavin Shan <gshan@redhat.com> wrote:
> 
> > Hi Mark,
> > 
> > On 4/24/20 8:11 PM, Mark Rutland wrote:  
> > > [Adding Steve, who added str_has_prefix()]
> > > 
> > > On Fri, Apr 24, 2020 at 02:53:14PM +1000, Gavin Shan wrote:    
> > >> The NUMA option is parsed by str_has_prefix() and the invalid option
> > >> like "numa=o" can be regarded as "numa=off" wrongly.    
> > > 
> > > Are you certain that can pass? If that can happen, str_has_prefix() is
> > > misnamed and does not seem to do what its kerneldoc says it does, as
> > > "off" is not a prefix of "o".
> > >     
> > 
> > Yes, It's possible. str_has_prefix() depends on strncmp(). In this particular
> > case, it's equal to the snippet of code as below: strncmp() returns zero.
> > str_has_prefix() returns 3.  
> 
> Wait! strncmp("o", "off", 3) returns zero?
> 
> That to me looks like a bug!
> 
> This means str_has_prefix() is broken in other areas as well.
> 
> 
> > 
> > int strncmp(const char *cs, const char *ct, size_t count)
> > {
> >          unsigned char c1, c2;
> > 
> >          while (count) {
> >                  c1 = *cs++;
> >                  c2 = *ct++;
> >                  if (c1 != c2)
> >                          return c1 < c2 ? -1 : 1;
> >                  if (!c1)                             /* break after first character is compared */  
> 
> Crap! That is totally wrong!

Looking at this again, it's not wrong. But how did we get here if c2 isn't
zero as well?

-- Steve

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Steven Rostedt]

On Mon, 27 Apr 2020 22:59:44 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 27 Apr 2020 22:54:06 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 28 Apr 2020 10:59:14 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> > > Hi Mark,
> > > 
> > > On 4/24/20 8:11 PM, Mark Rutland wrote:    
> > > > [Adding Steve, who added str_has_prefix()]
> > > > 
> > > > On Fri, Apr 24, 2020 at 02:53:14PM +1000, Gavin Shan wrote:      
> > > >> The NUMA option is parsed by str_has_prefix() and the invalid option
> > > >> like "numa=o" can be regarded as "numa=off" wrongly.      
> > > > 
> > > > Are you certain that can pass? If that can happen, str_has_prefix() is
> > > > misnamed and does not seem to do what its kerneldoc says it does, as
> > > > "off" is not a prefix of "o".
> > > >       
> > > 
> > > Yes, It's possible. str_has_prefix() depends on strncmp(). In this particular
> > > case, it's equal to the snippet of code as below: strncmp() returns zero.
> > > str_has_prefix() returns 3.    
> > 
> > Wait! strncmp("o", "off", 3) returns zero?
> > 
> > That to me looks like a bug!
> > 
> > This means str_has_prefix() is broken in other areas as well.
> > 
> >   
> > > 
> > > int strncmp(const char *cs, const char *ct, size_t count)
> > > {
> > >          unsigned char c1, c2;
> > > 
> > >          while (count) {
> > >                  c1 = *cs++;
> > >                  c2 = *ct++;
> > >                  if (c1 != c2)
> > >                          return c1 < c2 ? -1 : 1;
> > >                  if (!c1)                             /* break after first character is compared */    
> > 
> > Crap! That is totally wrong!  
> 
> Looking at this again, it's not wrong. But how did we get here if c2 isn't
> zero as well?
> 

Could this be a bug in the implementation of strncmp() in
arch/arm64/lib/strncmp.S. As I don't know arm64 assembly, I have no idea
what it is trying to do.

But strncmp("o","off",3) returning zero *is* a bug.

-- Steve

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Gavin Shan]

Hi Steven and Mark,

On 4/28/20 1:09 PM, Steven Rostedt wrote:

[...]

> 
> Could this be a bug in the implementation of strncmp() in
> arch/arm64/lib/strncmp.S. As I don't know arm64 assembly, I have no idea
> what it is trying to do.
> 
> But strncmp("o","off",3) returning zero *is* a bug.
> 

I think it's false alarm. The patch has been in my local repo for a while.
I checked out 5.7.rc3 and tried passing "numa=o" to the kernel, @numa_off
is unchanged and its value is false. I also check the return value from
strncmp() as below, it's correct. Nothing is broken. I should have retested
before posting it. Sorry for the noise. Please ignore the crap patch :)

diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index 4decf1659700..a8e5c6f7ba25 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c
@@ -32,6 +32,13 @@ static __init int numa_parse_early_param(char *opt)
         if (str_has_prefix(opt, "off"))
                 numa_off = true;
  
+       pr_info("numa_off=%s\n", numa_off ? "true" : "false");
+       pr_info("opt=%s\n", opt);
+       pr_info("len=%d\n", (int)strlen("off"));
+       pr_info("\n");
+       pr_info("================================\n");
+       pr_info("strncmp(opt, 'off', 3)=%d\n", strncmp(opt, "off", 3));
+

[    0.000000] NUMA: numa_off=false
[    0.000000] NUMA: opt=o
[    0.000000] NUMA: len=3
[    0.000000] NUMA:
[    0.000000] NUMA: ================================
[    0.000000] NUMA: strncmp(opt, 'off', 3)=-102

Thanks,
Gavin

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Will Deacon]

On Tue, Apr 28, 2020 at 02:35:20PM +1000, Gavin Shan wrote:
> On 4/28/20 1:09 PM, Steven Rostedt wrote:
> 
> [...]
> 
> > 
> > Could this be a bug in the implementation of strncmp() in
> > arch/arm64/lib/strncmp.S. As I don't know arm64 assembly, I have no idea
> > what it is trying to do.
> > 
> > But strncmp("o","off",3) returning zero *is* a bug.
> > 
> 
> I think it's false alarm. The patch has been in my local repo for a while.
> I checked out 5.7.rc3 and tried passing "numa=o" to the kernel, @numa_off
> is unchanged and its value is false. I also check the return value from
> strncmp() as below, it's correct. Nothing is broken. I should have retested
> before posting it. Sorry for the noise. Please ignore the crap patch :)

Hmm, it's still worrying that you had that patch kicking around though, as
it sounds like it /used/ to be broken. Would you be able to test the LTS
kernels (5.4, 4.19, 4.14, 4.9, 4.4) to check that we're not missing a
backport, please? Sorry to be a pain, but I'd like to get to the bottom of
this!

Thanks,

Will

Re: [PATCH] arm64/mm: Reject invalid NUMA option

[Gavin Shan]

Hi Will,

On 4/28/20 5:25 PM, Will Deacon wrote:
> On Tue, Apr 28, 2020 at 02:35:20PM +1000, Gavin Shan wrote:
>> On 4/28/20 1:09 PM, Steven Rostedt wrote:
>>
>> [...]
>>
>>>
>>> Could this be a bug in the implementation of strncmp() in
>>> arch/arm64/lib/strncmp.S. As I don't know arm64 assembly, I have no idea
>>> what it is trying to do.
>>>
>>> But strncmp("o","off",3) returning zero *is* a bug.
>>>
>>
>> I think it's false alarm. The patch has been in my local repo for a while.
>> I checked out 5.7.rc3 and tried passing "numa=o" to the kernel, @numa_off
>> is unchanged and its value is false. I also check the return value from
>> strncmp() as below, it's correct. Nothing is broken. I should have retested
>> before posting it. Sorry for the noise. Please ignore the crap patch :)
> 
> Hmm, it's still worrying that you had that patch kicking around though, as
> it sounds like it /used/ to be broken. Would you be able to test the LTS
> kernels (5.4, 4.19, 4.14, 4.9, 4.4) to check that we're not missing a
> backport, please? Sorry to be a pain, but I'd like to get to the bottom of
> this!
> 

Sure, There are nothing to worry. I tried the following branches of the stable
tree. They all looks good in this regard.

    linux-5.6.y
    linux-5.5.y
    linux-5.4.y
    linux-5.3.y
    linux-4.19.y
    linux-4.9.y

linux-4.4.y isn't tried because the corresponding parameter starts to exist
from linux-4.7.y: 1a2db300348b ("arm64, numa: Add NUMA support for arm64 platforms.")

    # git tag --contains 1a2db300348b | sort -V | head -n 3
    v4.7
    v4.7-rc1
    v4.7-rc2

In the experiment, the following pr_info() is added and I got same output
from above branches:

diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index b1e42bad69ac..1e0e3491de54 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c
@@ -44,6 +44,8 @@ static __init int numa_parse_early_param(char *opt)
         if (!strncmp(opt, "off", 3))
                 numa_off = true;
  
+       pr_info("===> numa_off=%s\n", numa_off ? "true" : "false");
+


[    0.000000] NUMA: ===> numa_off=false



There is unrelated compiling warnings in linux-5.3.y:

drivers/pinctrl/pinctrl-rockchip.c: In function ‘rockchip_gpio_set_config’:
drivers/pinctrl/pinctrl-rockchip.c:2783:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
    rockchip_gpio_set_debounce(gc, offset, true);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pinctrl/pinctrl-rockchip.c:2795:2: note: here
   default:
   ^~~~~~~


> Thanks,
> 
> Will
> 

Thanks,
Gavin