Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Reply-To: Gavin Shan <gshan@redhat.com>
Subject: Re: [PATCH] arm64/mm: Reject invalid NUMA option
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     catalin.marinas@arm.com, linux-kernel@vger.kernel.org,
        Steven Rostedt <rostedt@goodmis.org>, shan.gavin@gmail.com,
        will@kernel.org, linux-arm-kernel@lists.infradead.org
References: <20200424045314.16017-1-gshan@redhat.com>
 <20200424101132.GC1167@C02TD0UTHF1T.local>
From:   Gavin Shan <gshan@redhat.com>
Message-ID: <f83c0ce1-b1b2-31f4-60c8-15567b87a8ff@redhat.com>
Date:   Tue, 28 Apr 2020 10:59:14 +1000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.0
MIME-Version: 1.0
In-Reply-To: <20200424101132.GC1167@C02TD0UTHF1T.local>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>