Re: [PATCH v2] MIPS: Avoid VDSO ABI breakage due to global register variable

[Paul Burton]

Hi Vincenzo,

On Thu, Jan 02, 2020 at 04:56:00PM +0000, Vincenzo Frascino wrote:
> Hi Paul,
> 
> Happy new year.

Thanks; happy new year to you too!

> On 02/01/2020 04:50, Paul Burton wrote:
> > Declaring __current_thread_info as a global register variable has the
> > effect of preventing GCC from saving & restoring its value in cases
> > where the ABI would typically do so.
> > 
> > To quote GCC documentation:
> > 
> >> If the register is a call-saved register, call ABI is affected: the
> >> register will not be restored in function epilogue sequences after the
> >> variable has been assigned. Therefore, functions cannot safely return
> >> to callers that assume standard ABI.
> > 
> > When our position independent VDSO is built for the n32 or n64 ABIs all
> > functions it exposes should be preserving the value of $gp/$28 for their
> > caller, but in the presence of the __current_thread_info global register
> > variable GCC stops doing so & simply clobbers $gp/$28 when calculating
> > the address of the GOT.
> > 
> > In cases where the VDSO returns success this problem will typically be
> > masked by the caller in libc returning & restoring $gp/$28 itself, but
> > that is by no means guaranteed. In cases where the VDSO returns an error
> > libc will typically contain a fallback path which will now fail
> > (typically with a bad memory access) if it attempts anything which
> > relies upon the value of $gp/$28 - eg. accessing anything via the GOT.
> > 
> 
> First of all good catch. I just came back from holidays and seems that you guys
> had fun without me :)

The fun never stops ;)

> Did you consider the option to use "-ffixed-gp -ffixed-28" as a compilation
> flags for the vDSO library (Arvind was mentioning it as well in his reply)?
> According to the GCC manual "treats the register as a fixed register; generated
> code should never refer to it"
> (https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#Code-Gen-Options)
> 
> I did some experiments this morning an seems that on MIPS the convention is not
> honored at least on GCC-9. Do you know who to contact to get it enabled/fixed in
> the compiler?
> 
> With this:
> 
> Reviewed-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> Tested-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>

Using -ffixed-gp wouldn't be correct for the VDSO - the VDSO itself is
position independent code, and will need to use $gp to access the GOT
which is part of how position-independence is achieved (technically you
could access the GOT using another register of course but you'd need
some way to persuade the compiler to break with convention & you'd gain
nothing meaningful since you'd need to use some other register anyway).
If we use -ffixed-gp then we're telling GCC not to use $gp, and that
doesn't make sense. If we consider -ffixed-gp as telling GCC not to use
$gp as a general purpose register then it's meaningless because $gp
already has a specific use & isn't used as a general purpose register.
If we consider -ffixed-gp as telling GCC not to use $gp at all then it
doesn't make sense because it needs to in order to access the GOT.

In terms of GCC's flags we'd want to use -fcall-saved-gp, but that would
just be telling GCC information it already knows about the n32 & n64
ABIs & indeed it seems to have no effect at all on the way GCC handles
the global register variable - it doesn't cause gcc to save & restore
$gp with the global register variable present, so you gain nothing.

We could use -ffixed-gp for the kernel proper (& not the VDSO), but:

1) The kernel builds as non-PIC code with no $gp-based optimizations
   enabled, and since this has been fine forever it seems safe to expect
   the compiler not to start using $gp in new ways.

2) It would be a separate issue to fixing the VDSO anyway.

So I'll go ahead with the v2 patch without changing compiler flags for
now.

Thanks,
    Paul

> > One fix for this would be to move the declaration of
> > __current_thread_info inside the current_thread_info() function,
> > demoting it from global register variable to local register variable &
> > avoiding inadvertently creating a non-standard calling ABI for the VDSO.
> > Unfortunately this causes issues for clang, which doesn't support local
> > register variables as pointed out by commit fe92da0f355e ("MIPS: Changed
> > current_thread_info() to an equivalent supported by both clang and GCC")
> > which introduced the global register variable before we had a VDSO to
> > worry about.
> > 
> > Instead, fix this by continuing to use the global register variable for
> > the kernel proper but declare __current_thread_info as a simple extern
> > variable when building the VDSO. It should never be referenced, and will
> > cause a link error if it is. This resolves the calling convention issue
> > for the VDSO without having any impact upon the build of the kernel
> > itself for either clang or gcc.
> > 
> > Signed-off-by: Paul Burton <paulburton@xxxxxxxxxx>
> > Reported-by: "Jason A. Donenfeld" <Jason@xxxxxxxxx>
> > Fixes: ebb5e78cc634 ("MIPS: Initial implementation of a VDSO")
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Cc: Christian Brauner <christian.brauner@xxxxxxxxxxxxx>
> > Cc: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v4.4+
> > ---
> > Changes in v2:
> > - Switch to the #ifdef __VDSO__ approach rather than using a local
> >   register variable which clang doesn't support.
> > ---
> >  arch/mips/include/asm/thread_info.h | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
> > index 4993db40482c..ee26f9a4575d 100644
> > --- a/arch/mips/include/asm/thread_info.h
> > +++ b/arch/mips/include/asm/thread_info.h
> > @@ -49,8 +49,26 @@ struct thread_info {
> >  	.addr_limit	= KERNEL_DS,		\
> >  }
> >  
> > -/* How to get the thread information struct from C.  */
> > +/*
> > + * A pointer to the struct thread_info for the currently executing thread is
> > + * held in register $28/$gp.
> > + *
> > + * We declare __current_thread_info as a global register variable rather than a
> > + * local register variable within current_thread_info() because clang doesn't
> > + * support explicit local register variables.
> > + *
> > + * When building the VDSO we take care not to declare the global register
> > + * variable because this causes GCC to not preserve the value of $28/$gp in
> > + * functions that change its value (which is common in the PIC VDSO when
> > + * accessing the GOT). Since the VDSO shouldn't be accessing
> > + * __current_thread_info anyway we declare it extern in order to cause a link
> > + * failure if it's referenced.
> > + */
> > +#ifdef __VDSO__
> > +extern struct thread_info *__current_thread_info;
> > +#else
> >  register struct thread_info *__current_thread_info __asm__("$28");
> > +#endif
> >  
> >  static inline struct thread_info *current_thread_info(void)
> >  {
> > 
> 
> -- 
> Regards,
> Vincenzo