linux-kernel - Re: [PATCH v2 1/3] x86/fred: Parse cmdline param "fred=" in cpu_parse_early

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e0de0183-00d1-46a5-ba2b-6df431b58743@suse.com>
Date: Mon, 15 Jul 2024 09:44:25 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Xin Li <xin@...or.com>, linux-kernel@...r.kernel.org
Cc: hpa@...or.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, peterz@...radead.org,
 andrew.cooper3@...rix.com, houwenlong.hwl@...group.com
Subject: Re: [PATCH v2 1/3] x86/fred: Parse cmdline param "fred=" in
 cpu_parse_early_param()



On 12.07.24 г. 20:40 ч., Xin Li wrote:
> On 7/10/2024 11:53 AM, Nikolay Borisov wrote:
>> On 9.07.24 г. 18:40 ч., Xin Li (Intel) wrote:
> 
>>> @@ -1510,6 +1510,11 @@ static void __init cpu_parse_early_param(void)
>>>       if (cmdline_find_option_bool(boot_command_line, "nousershstk"))
>>>           setup_clear_cpu_cap(X86_FEATURE_USER_SHSTK);
>>> +    /* Minimize the gap between FRED is available and available but 
>>> disabled. */
>>> +    arglen = cmdline_find_option(boot_command_line, "fred", arg, 
>>> sizeof(arg));
>>> +    if (arglen != 2 || strncmp(arg, "on", 2))
>>
>> I'm confused why you keep perverting the calling convention of 
>> cmdline_find_option. The doc clearly states:
>>
>>      * Returns the position of that @option (starts counting with 1)
>>      * or 0 on not found.  @option will only be found if it is found
>>      * as an entire word in @cmdline.  For instance, if @option="car"
>>      * then a cmdline which contains "cart" will not match.
>>
>> You should only care if arglen is non 0, which if it is you check if 
>> its value equal 'on', why bother with its starting position?
>>


Actually, I have quoted the wrong doc, the correct one is:

"
Returns the length of the argument (regardless of if it was
truncated to fit in the buffer), or -1 on not found.
"

> 
> Well, just look at how it is used in match_option() in
> arch/x86/kernel/cpu/bugs.c and arch/x86/kernel/cpu/intel.c.

Exactly, in bugs.c it's used as I've suggested:

In spectre_v2_parse_user_cmdline it checks if spectre_v2_user is present 
(if a negative value is returned) and if not it returns some default.

In spectre_v2_parse_cmdline it's used exactly the same way - return some 
default if that function returns a negative value (spectre_v2 check) or 
return some specific value if it found nospectre_v2.

And in sld_state_setup the code just checks for a non-negative value i.e 
the argument has been found.

Otoh, I see what you are trying to say if I look at the usage of this 
function in arch/x86/boot/compressed/acpi.c


Still I find this convention a bit counter-intuitive, but given it's not 
a precedent I'm fine with leaving it as is.




> 
> This is a short version and it will be expanded once we have more
> option strings well defined (match_option() should be a common lib
> function then).
>