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
