linux-kernel - Re: [v5 PATCH] RISC-V: Fix unsupported isa string info.

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: <3fb8d4f0383b005ecd932a69c4dd295a79b6fb1a.camel@wdc.com>
Date:   Fri, 16 Aug 2019 19:21:52 +0000
From:   Atish Patra <Atish.Patra@....com>
To:     "hch@...radead.org" <hch@...radead.org>
CC:     "anup@...infault.org" <anup@...infault.org>,
        "paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "palmer@...ive.com" <palmer@...ive.com>,
        "aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
        "johan@...nel.org" <johan@...nel.org>
Subject: Re: [v5 PATCH] RISC-V: Fix unsupported isa string info.

On Mon, 2019-08-12 at 08:02 -0700, Christoph Hellwig wrote:
> > +	for (e = mandatory_ext; *e != '\0'; ++e) {
> > +		if (isa[0] != e[0]) {
> > +#if defined(CONFIG_FP)
> > +			if ((isa[0] == 'f') || (isa[0] == 'd'))
> > +				continue;
> > +#endif
> > +			unsupported_isa[index] = e[0];
> > +			index++;
> > +		}
> 
> I'd just use if (IS_ENABLED()) here to get full compiler coverage.
> Also no need for the inner braces.
> 

Sure. I will do that.

> > +	if (isa[0] != '\0') {
> > +		/* Add remainging isa strings */
> > +		for (e = isa; *e != '\0'; ++e) {
> > +#if !defined(CONFIG_VIRTUALIZATION)
> > +			if (e[0] != 'h')
> > +#endif
> > +				seq_write(f, e, 1);
> > +		}
> > +	}
> 
> This one I don't get.  Why do we want to check CONFIG_VIRTUALIZATION?
> 

If CONFIG_VIRTUALIZATION is not enabled, it shouldn't print that
hypervisor extension "h" in isa extensions.

This can be extended to any other future extensions and related config.

> >  	seq_puts(f, "\n");ther you want to know if a specific extension
> > is enabled
> >  
> >  	/*
> >  	 * If we were given an unsupported ISA in the device tree then
> > print
> >  	 * a bit of info describing what went wrong.
> >  	 */
> > -	if (isa[0] != '\0')
> > -		pr_info("unsupported ISA \"%s\" in device tree\n",
> > orig_isa);
> > +	if (unsupported_isa[0])
> > +		pr_info("unsupported ISA extensions \"%s\" in device
> > tree for cpu [%ld]\n",
> > +			unsupported_isa, cpuid);
> 
> And I'm not even sure why we care about unsupported
> extensions.  Sooner
> or late a few will op up and they should be harmless.

This is just an information to the userspace that some of the mandatory
ISA extensions ("mafdcsu") are not supported in kernel which may lead
to undesirable results.


Regards,
Atish