Hi Paul, Happy new year. 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 :) 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> > 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
Attachment:
pEpkey.asc
Description: application/pgp-keys
