linux-kernel - Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities.

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]

Date:   Wed, 13 Feb 2019 09:44:42 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Atish Patra <atish.patra@....com>
Cc:     Johan Hovold <johan@...nel.org>, Rob Herring <robh@...nel.org>,
        Albert Ou <aou@...s.berkeley.edu>,
        Jason Cooper <jason@...edaemon.net>,
        Alan Kao <alankao@...estech.com>,
        Dmitriy Cherkasov <dmitriy@...-tech.org>,
        Andreas Schwab <schwab@...e.de>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Palmer Dabbelt <palmer@...ive.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Anup Patel <anup@...infault.org>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [v4 PATCH 8/8] RISC-V: Assign hwcap as per comman capabilities.

On Tue, Feb 12, 2019 at 11:58:10AM -0800, Atish Patra wrote:
> On 2/12/19 3:25 AM, Johan Hovold wrote:
> > On Tue, Feb 12, 2019 at 03:10:12AM -0800, Atish Patra wrote:
> >> Currently, we set hwcap based on first valid hart from DT. This may not
> >> be correct always as that hart might not be current booting cpu or may
> >> have a different capability.
> >>
> >> Set hwcap as the capabilities supported by all possible harts with "okay"
> >> status.
> >>
> >> Signed-off-by: Atish Patra <atish.patra@....com>
> >> ---
> >>   arch/riscv/kernel/cpufeature.c | 41 ++++++++++++++++++++++-------------------
> >>   1 file changed, 22 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >> index e7a4701f..a1e4fb34 100644
> >> --- a/arch/riscv/kernel/cpufeature.c
> >> +++ b/arch/riscv/kernel/cpufeature.c
> >> @@ -20,6 +20,7 @@
> >>   #include <linux/of.h>
> >>   #include <asm/processor.h>
> >>   #include <asm/hwcap.h>
> >> +#include <asm/smp.h>
> >>   
> >>   unsigned long elf_hwcap __read_mostly;
> >>   #ifdef CONFIG_FPU
> >> @@ -42,28 +43,30 @@ void riscv_fill_hwcap(void)
> >>   
> >>   	elf_hwcap = 0;
> >>   
> >> -	/*
> >> -	 * We don't support running Linux on hertergenous ISA systems.  For
> >> -	 * now, we just check the ISA of the first "okay" processor.
> >> -	 */
> >>   	for_each_of_cpu_node(node) {
> >> -		if (riscv_of_processor_hartid(node) >= 0)
> >> -			break;
> >> -	}
> >> -	if (!node) {
> >> -		pr_warn("Unable to find \"cpu\" devicetree entry\n");
> >> -		return;
> >> -	}
> >> +		unsigned long this_hwcap = 0;
> >>   
> >> -	if (of_property_read_string(node, "riscv,isa", &isa)) {
> >> -		pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> >> -		of_node_put(node);
> >> -		return;
> >> -	}
> >> -	of_node_put(node);
> >> +		if (riscv_of_processor_hartid(node) < 0)
> >> +			continue;
> >>
> 
> >> -	for (i = 0; i < strlen(isa); ++i)
> >> -		elf_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
> >> +		if (of_property_read_string(node, "riscv,isa", &isa)) {
> >> +			pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> >> +			return;
> > 
> > Did you want "continue" here to continue processing the other harts?
> 
> Hmm. If a cpu node doesn't have isa in DT, that means DT is wrong. A 
> "continue" here will let user space use other harts just with a warning 
> message?
> 
> Returning here will not set elf_hwcap which forces the user to fix the 
> DT. I am not sure what should be the defined behavior in this case.
>
> Any thoughts ?

The problem is that the proposed code might still set elf_hwcap -- it
all depends on the order of the hart nodes in dt (i.e. it will only be
left unset if the first node is malformed).

For that reason, I'd say it's better to either bail out (hard or at
least with elf_hwcap unset) or to continue processing the other nodes.

The former might break current systems with malformed dt, though.

And since the harts are expected to have the same ISA, continuing the
processing while warning and ignoring the malformed node might be
acceptable.

Johan