* [PATCH] arm64/mm: Reject invalid NUMA option @ 2020-04-24 4:53 Gavin Shan 2020-04-24 10:11 ` Mark Rutland 0 siblings, 1 reply; 9+ messages in thread From: Gavin Shan @ 2020-04-24 4:53 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-kernel, catalin.marinas, will, mark.rutland, shan.gavin 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-24 4:53 [PATCH] arm64/mm: Reject invalid NUMA option Gavin Shan @ 2020-04-24 10:11 ` Mark Rutland 2020-04-28 0:59 ` Gavin Shan 0 siblings, 1 reply; 9+ messages in thread From: Mark Rutland @ 2020-04-24 10:11 UTC (permalink / raw) To: Gavin Shan Cc: linux-arm-kernel, linux-kernel, catalin.marinas, will, shan.gavin, Steven Rostedt [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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-24 10:11 ` Mark Rutland @ 2020-04-28 0:59 ` Gavin Shan 2020-04-28 2:54 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Gavin Shan @ 2020-04-28 0:59 UTC (permalink / raw) To: Mark Rutland Cc: catalin.marinas, linux-kernel, Steven Rostedt, shan.gavin, will, linux-arm-kernel 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 > ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-28 0:59 ` Gavin Shan @ 2020-04-28 2:54 ` Steven Rostedt 2020-04-28 2:59 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-04-28 2:54 UTC (permalink / raw) To: Gavin Shan Cc: Mark Rutland, catalin.marinas, linux-kernel, shan.gavin, will, linux-arm-kernel 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-28 2:54 ` Steven Rostedt @ 2020-04-28 2:59 ` Steven Rostedt 2020-04-28 3:09 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-04-28 2:59 UTC (permalink / raw) To: Gavin Shan Cc: Mark Rutland, catalin.marinas, linux-kernel, shan.gavin, will, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-28 2:59 ` Steven Rostedt @ 2020-04-28 3:09 ` Steven Rostedt 2020-04-28 4:35 ` Gavin Shan 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-04-28 3:09 UTC (permalink / raw) To: Gavin Shan Cc: Mark Rutland, catalin.marinas, linux-kernel, shan.gavin, will, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-28 3:09 ` Steven Rostedt @ 2020-04-28 4:35 ` Gavin Shan 2020-04-28 7:25 ` Will Deacon 0 siblings, 1 reply; 9+ messages in thread From: Gavin Shan @ 2020-04-28 4:35 UTC (permalink / raw) To: Steven Rostedt Cc: Mark Rutland, catalin.marinas, linux-kernel, shan.gavin, will, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-28 4:35 ` Gavin Shan @ 2020-04-28 7:25 ` Will Deacon 2020-04-28 8:56 ` Gavin Shan 0 siblings, 1 reply; 9+ messages in thread From: Will Deacon @ 2020-04-28 7:25 UTC (permalink / raw) To: Gavin Shan Cc: Steven Rostedt, Mark Rutland, catalin.marinas, linux-kernel, shan.gavin, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] arm64/mm: Reject invalid NUMA option 2020-04-28 7:25 ` Will Deacon @ 2020-04-28 8:56 ` Gavin Shan 0 siblings, 0 replies; 9+ messages in thread From: Gavin Shan @ 2020-04-28 8:56 UTC (permalink / raw) To: Will Deacon Cc: Steven Rostedt, Mark Rutland, catalin.marinas, linux-kernel, shan.gavin, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-28 8:56 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-24 4:53 [PATCH] arm64/mm: Reject invalid NUMA option Gavin Shan 2020-04-24 10:11 ` Mark Rutland 2020-04-28 0:59 ` Gavin Shan 2020-04-28 2:54 ` Steven Rostedt 2020-04-28 2:59 ` Steven Rostedt 2020-04-28 3:09 ` Steven Rostedt 2020-04-28 4:35 ` Gavin Shan 2020-04-28 7:25 ` Will Deacon 2020-04-28 8:56 ` Gavin Shan
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).