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] [day] [month] [year] [list]
Message-Id: <1553665102.ow7h62jw1u.astroid@bobo.none>
Date:   Wed, 27 Mar 2019 16:02:16 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Alastair D'Silva <alastair@....ibm.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Christophe Leroy <christophe.leroy@....fr>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        Mahesh
 Salgaonkar <mahesh@...ux.vnet.ibm.com>,
        Michal Hocko <mhocko@...e.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        Paul Mackerras <paulus@...ba.org>,
        Mike
 Rapoport <rppt@...ux.vnet.ibm.com>
Subject: Re: [PATCH v2]  arch/powerpc: Rework local_paca to avoid LTO warnings

Alastair D'Silva's on March 27, 2019 2:37 pm:
> On Tue, 2019-03-26 at 15:58 +1000, Nicholas Piggin wrote:
>> Alastair D'Silva's on March 14, 2019 12:31 pm:
>> > From: Alastair D'Silva <alastair@...ilva.org>
>> > 
>> > When building an LTO kernel, the existing code generates warnings:
>> >     ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
>> >         ‘local_paca’ used for multiple global register variables
>> >      register struct paca_struct *local_paca asm("r13");
>> >                                   ^
>> >     ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
>> >         ‘local_paca’
>> 
>> Isn't this a bogus warning? It doesn't look like there's a way to 
>> define it any other way.
> 
> There isn't any other way to define it as a global. However, the
> warning is legitimate.
> 
> The compiler sees that there are multiple global register variables,
> all pointing at the same register.

It's one variable though, so it's not a legitimate warning (unless
there is a way to declare a "reference" to it that we are not doing).

> 
> The compiler can only determine this when LTO is used, as otherwise it
> only sees the one in the current compilation unit, whicd disappears by
> the time the kernel is linked.
> 
>> 
>> > This patch reworks local_paca into an inline getter & setter
>> > function,
>> > which addresses the warning.
>> > 
>> > Changelog:
>> > V2
>> >   - Address whitespace issues
>> >   - keep new implementation close to where the old implementation
>> > was
>> > 
>> > Signed-off-by: Alastair D'Silva <alastair@...ilva.org>
>> > ---
>> >  arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--
>> > ------
>> >  arch/powerpc/kernel/paca.c      |  2 +-
>> >  2 files changed, 29 insertions(+), 10 deletions(-)
>> > 
>> > diff --git a/arch/powerpc/include/asm/paca.h
>> > b/arch/powerpc/include/asm/paca.h
>> > index e843bc5d1a0f..2fa0b43357c9 100644
>> > --- a/arch/powerpc/include/asm/paca.h
>> > +++ b/arch/powerpc/include/asm/paca.h
>> > @@ -34,19 +34,38 @@
>> >  #include <asm/cpuidle.h>
>> >  #include <asm/atomic.h>
>> >  
>> > -register struct paca_struct *local_paca asm("r13");
>> > -
>> >  #if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
>> >  extern unsigned int debug_smp_processor_id(void); /* from
>> > linux/smp.h */
>> > -/*
>> > - * Add standard checks that preemption cannot occur when using
>> > get_paca():
>> > - * otherwise the paca_struct it points to may be the wrong one
>> > just after.
>> > - */
>> > -#define get_paca()	((void) debug_smp_processor_id(), local_paca)
>> > -#else
>> > -#define get_paca()	local_paca
>> >  #endif
>> >  
>> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
>> > +{
>> > +	register struct paca_struct *paca asm("r13");
>> > +
>> > +	return paca;
>> > +}
>> 
>> Problem is it now changes the global register variable to a local 
>> register variable. The compiler would presumably be within its rights
>> to "cache" that return value or use another register for it, which
>> is not really what we want.
>> 
>> 
> 
> I've confirmed that at least with GCC 8.2.0, the generated assembler is
> similar, but yes, the compiler may be free to take a copy into another
> register (although that would be a terrible optimisation), and then
> operate on that value.

Yep.

> 
> Subsequent uses would still have to call the function (ie. fetch the
> data from r13) regardless, so I believe this scenario is safe.
> 
> Can you think of a scenario where this is a problem?

It wouldn't be the subsequent use but the one use. If you're preempted
then you can't be using a stale r13 value.

Quite possibly any bug like that would already be buggy now, the cases
where it matters tend to need asm to access it. And it's something we
need to really audit and have proper accessors and preempt warnings
that Ben always harped on about.

But to just work around this warning it seems pretty dangerous to
change this and hope.

Thanks,
Nick

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ