linux-kernel - Re: [PATCH v9 03/17] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access

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: <alpine.DEB.2.21.1906262109200.32342@nanos.tec.linutronix.de>
Date:   Wed, 26 Jun 2019 21:15:23 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     David Laight <David.Laight@...LAB.COM>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krcmar <rkrcmar@...hat.com>,
        Christopherson Sean J <sean.j.christopherson@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>,
        Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH v9 03/17] x86/split_lock: Align x86_capability to unsigned
 long to avoid split locked access

On Tue, 25 Jun 2019, Fenghua Yu wrote:
> On Mon, Jun 24, 2019 at 03:12:49PM +0000, David Laight wrote:
> > > @@ -93,7 +93,9 @@ struct cpuinfo_x86 {
> > >  	__u32			extended_cpuid_level;
> > >  	/* Maximum supported CPUID level, -1=no CPUID: */
> > >  	int			cpuid_level;
> > > -	__u32			x86_capability[NCAPINTS + NBUGINTS];
> > > +	/* Aligned to size of unsigned long to avoid split lock in atomic ops */
> > 
> > Wrong comment.
> > Something like:
> > 	/* Align to sizeof (unsigned long) because the array is passed to the
> > 	 * atomic bit-op functions which require an aligned unsigned long []. */
> 
> The problem we try to fix here is not because "the array is passed to the
> atomic bit-op functions which require an aligned unsigned long []".
> 
> The problem is because of the possible split lock issue. If it's not because
> of split lock issue, there is no need to have this patch.
> 
> So I would think my comment is right to point out explicitly why we need
> this alignment.

The underlying problem why you need that alignemnt is that the invocation
of the bitops does a type cast. And that's independent of split lock. Split
lock makes the problem visible. So the alignment papers over that. And
while this 'works' in x86 it's fundamentaly broken on big endian. So no,
your comment is not right to the point because it gives the wrong
information.

> > 
> > > +	__u32			x86_capability[NCAPINTS + NBUGINTS]
> > > +				__aligned(sizeof(unsigned long));
> > 
> > It might be better to use a union (maybe unnamed) here.
> 
> That would be another patch. This patch just simply fixes the split lock
> issue.

Why? That's a straight forward and obvious fix and way better than these
alignment games. It's still wrong for BE....

So anyway, this wants a comment which explains the underlying issue and not
a comment which blurbs about split locks.

Thanks,

	tglx