[PATCH] MIPS: Don't declare __current_thread_info globally

[PATCH] MIPS: Don't declare __current_thread_info globally

[Paul Burton]

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.

Fix this by moving 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.

Signed-off-by: Paul Burton <paulburton@kernel.org>
Reported-by: "Jason A. Donenfeld" <Jason@zx2c4.com>
Fixes: ebb5e78cc634 ("MIPS: Initial implementation of a VDSO")
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christian Brauner <christian.brauner@canonical.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: <stable@vger.kernel.org> # v4.4+
---
 arch/mips/include/asm/thread_info.h | 4 ++--
 arch/mips/kernel/relocate.c         | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
index 4993db40482c..aceefc3f9a1a 100644
--- a/arch/mips/include/asm/thread_info.h
+++ b/arch/mips/include/asm/thread_info.h
@@ -50,10 +50,10 @@ struct thread_info {
 }
 
 /* How to get the thread information struct from C.  */
-register struct thread_info *__current_thread_info __asm__("$28");
-
 static inline struct thread_info *current_thread_info(void)
 {
+	register struct thread_info *__current_thread_info __asm__("$28");
+
 	return __current_thread_info;
 }
 
diff --git a/arch/mips/kernel/relocate.c b/arch/mips/kernel/relocate.c
index 3d80a51256de..c9afdc39b003 100644
--- a/arch/mips/kernel/relocate.c
+++ b/arch/mips/kernel/relocate.c
@@ -296,6 +296,7 @@ static inline int __init relocation_addr_valid(void *loc_new)
 
 void *__init relocate_kernel(void)
 {
+	register struct thread_info *__current_thread_info __asm__("$28");
 	void *loc_new;
 	unsigned long kernel_length;
 	unsigned long bss_length;
-- 
2.24.0

Re: [PATCH] MIPS: Don't declare __current_thread_info globally

[Arnd Bergmann]

On Wed, Jan 1, 2020 at 6:57 PM Paul Burton <paulburton@kernel.org> wrote:
> diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
> index 4993db40482c..aceefc3f9a1a 100644
> --- a/arch/mips/include/asm/thread_info.h
> +++ b/arch/mips/include/asm/thread_info.h
> @@ -50,10 +50,10 @@ struct thread_info {
>  }
>
>  /* How to get the thread information struct from C.  */
> -register struct thread_info *__current_thread_info __asm__("$28");
> -
>  static inline struct thread_info *current_thread_info(void)
>  {
> +       register struct thread_info *__current_thread_info __asm__("$28");
> +
>         return __current_thread_info;
>  }

This looks like a nice fix, but are you sure it doesn't allow the compiler to
reuse $28 for another purpose in the kernel under register pressure,
which would break current_thread_info()?

I see in the MIPS ABI document that $28 is preserved across function
calls, but I don't see any indication that a function is not allowed
to modify it and later restore the original content.

        Arnd

Re: [PATCH] MIPS: Don't declare __current_thread_info globally

[Arvind Sankar]

On Wed, Jan 01, 2020 at 09:51:02PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 1, 2020 at 6:57 PM Paul Burton <paulburton@kernel.org> wrote:
> > diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
> > index 4993db40482c..aceefc3f9a1a 100644
> > --- a/arch/mips/include/asm/thread_info.h
> > +++ b/arch/mips/include/asm/thread_info.h
> > @@ -50,10 +50,10 @@ struct thread_info {
> >  }
> >
> >  /* How to get the thread information struct from C.  */
> > -register struct thread_info *__current_thread_info __asm__("$28");
> > -
> >  static inline struct thread_info *current_thread_info(void)
> >  {
> > +       register struct thread_info *__current_thread_info __asm__("$28");
> > +
> >         return __current_thread_info;
> >  }
> 
> This looks like a nice fix, but are you sure it doesn't allow the compiler to
> reuse $28 for another purpose in the kernel under register pressure,
> which would break current_thread_info()?
> 
> I see in the MIPS ABI document that $28 is preserved across function
> calls, but I don't see any indication that a function is not allowed
> to modify it and later restore the original content.
> 
>         Arnd

The compiler can already do that even with a global definition.

The doc since gcc 9 [1] says:

"Accesses to the variable may be optimized as usual and the register
remains available for allocation and use in any computations, provided
that observable values of the variable are not affected."

and

"Furthermore, since the register is not reserved exclusively for the
variable, accessing it from handlers of asynchronous signals may observe
unrelated temporary values residing in the register."

I'm not sure if this was a change in gcc 9 or simply the doc was wrong
earlier.

Should there be a -ffixed-28 cflag for MIPS? alpha and hexagon seem to
have that and they also keep current_thread_info in a register.

Also, commit fe92da0f355e9 ("MIPS: Changed current_thread_info() to an
equivalent supported by both clang and GCC") moved this from local to
global because local apparently didn't work on clang?

[1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Global-Register-Variables.html

Re: [PATCH] MIPS: Don't declare __current_thread_info globally

[Nathan Chancellor]

On Wed, Jan 01, 2020 at 07:53:45PM -0500, Arvind Sankar wrote:
> On Wed, Jan 01, 2020 at 09:51:02PM +0100, Arnd Bergmann wrote:
> > On Wed, Jan 1, 2020 at 6:57 PM Paul Burton <paulburton@kernel.org> wrote:
> > > diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
> > > index 4993db40482c..aceefc3f9a1a 100644
> > > --- a/arch/mips/include/asm/thread_info.h
> > > +++ b/arch/mips/include/asm/thread_info.h
> > > @@ -50,10 +50,10 @@ struct thread_info {
> > >  }
> > >
> > >  /* How to get the thread information struct from C.  */
> > > -register struct thread_info *__current_thread_info __asm__("$28");
> > > -
> > >  static inline struct thread_info *current_thread_info(void)
> > >  {
> > > +       register struct thread_info *__current_thread_info __asm__("$28");
> > > +
> > >         return __current_thread_info;
> > >  }
> > 
> > This looks like a nice fix, but are you sure it doesn't allow the compiler to
> > reuse $28 for another purpose in the kernel under register pressure,
> > which would break current_thread_info()?
> > 
> > I see in the MIPS ABI document that $28 is preserved across function
> > calls, but I don't see any indication that a function is not allowed
> > to modify it and later restore the original content.
> > 
> >         Arnd
> 
> The compiler can already do that even with a global definition.
> 
> The doc since gcc 9 [1] says:
> 
> "Accesses to the variable may be optimized as usual and the register
> remains available for allocation and use in any computations, provided
> that observable values of the variable are not affected."
> 
> and
> 
> "Furthermore, since the register is not reserved exclusively for the
> variable, accessing it from handlers of asynchronous signals may observe
> unrelated temporary values residing in the register."
> 
> I'm not sure if this was a change in gcc 9 or simply the doc was wrong
> earlier.
> 
> Should there be a -ffixed-28 cflag for MIPS? alpha and hexagon seem to
> have that and they also keep current_thread_info in a register.
> 
> Also, commit fe92da0f355e9 ("MIPS: Changed current_thread_info() to an
> equivalent supported by both clang and GCC") moved this from local to
> global because local apparently didn't work on clang?
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Global-Register-Variables.html

Yeah this patch appears to break booting malta_defconfig in QEMU when
built with clang; additionally, there are a TON of warnings about this
variable being uninitialized:

../arch/mips/include/asm/thread_info.h:57:9: warning: variable '__current_thread_info' is uninitialized when used here [-Wuninitialized]
        return __current_thread_info;
               ^~~~~~~~~~~~~~~~~~~~~
../arch/mips/include/asm/thread_info.h:55:52: note: initialize the variable '__current_thread_info' to silence this warning
        register struct thread_info *__current_thread_info __asm__("$28");
                                                          ^
                                                           = NULL
1 warning generated.

Seems like this is expected according to that previous commit? I
noticed there is another instance in arch/mips but it doesn't appear to
affect everything.

https://github.com/ClangBuiltLinux/linux/issues/606

Cheers,
Nathan

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

[Paul Burton]

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.

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@kernel.org>
Reported-by: "Jason A. Donenfeld" <Jason@zx2c4.com>
Fixes: ebb5e78cc634 ("MIPS: Initial implementation of a VDSO")
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christian Brauner <christian.brauner@canonical.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: <stable@vger.kernel.org> # 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)
 {
-- 
2.24.1

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

[Jason A. Donenfeld]

Thanks, looks good to me:

Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
    or
Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>

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

[Vincenzo Frascino]

[-- Attachment #1: Type: text/plain, Size: 5162 bytes --]

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@arm.com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

> 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@kernel.org>
> Reported-by: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Fixes: ebb5e78cc634 ("MIPS: Initial implementation of a VDSO")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Christian Brauner <christian.brauner@canonical.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: <stable@vger.kernel.org> # 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 #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 14291 bytes --]

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@arm.com>
> Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

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@kernel.org>
> > Reported-by: "Jason A. Donenfeld" <Jason@zx2c4.com>
> > Fixes: ebb5e78cc634 ("MIPS: Initial implementation of a VDSO")
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Christian Brauner <christian.brauner@canonical.com>
> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > Cc: <stable@vger.kernel.org> # 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

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

[Vincenzo Frascino]

Hi Paul,

On 1/3/20 12:42 AM, Paul Burton wrote:
> 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.

Makes totally sense. Thanks for the explanation.

-- 
Regards,
Vincenzo

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

[Paul Burton]

Hello,

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.
> 
> 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.

Applied to mips-fixes.

> commit bbcc5672b006
> https://git.kernel.org/mips/c/bbcc5672b006
> 
> Signed-off-by: Paul Burton <paulburton@kernel.org>
> Fixes: ebb5e78cc634 ("MIPS: Initial implementation of a VDSO")
> Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>

Thanks,
    Paul

[ This message was auto-generated; if you believe anything is incorrect
  then please email paulburton@kernel.org to report it. ]