Looking at generated code for handle_signal32() shows calls to a
function called __unsafe_save_user_regs.constprop.0 while user access
is open.

And that __unsafe_save_user_regs.constprop.0 function has two nops at
the begining, allowing it to be traced, which is unexpected during
user access open window.

The solution could be to mark __unsafe_save_user_regs() no trace, but
to be on the safe side the most efficient is to flag it __always_inline
as already done for function __unsafe_restore_general_regs(). The
function is relatively small and only called twice, so the size
increase will remain in the noise.

Do the same with save_tm_user_regs_unsafe() as it may suffer the
same issue.

Fixes: ef75e7318294 ("powerpc/signal32: Transform save_user_regs() and save_tm_user_regs() in 'unsafe' version")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/signal_32.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index c114c7f25645..7a718ed32b27 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -264,8 +264,9 @@ static void prepare_save_user_regs(int ctx_has_vsx_region)
 #endif
 }
 
-static int __unsafe_save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
-				   struct mcontext __user *tm_frame, int ctx_has_vsx_region)
+static __always_inline int
+__unsafe_save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
+			struct mcontext __user *tm_frame, int ctx_has_vsx_region)
 {
 	unsigned long msr = regs->msr;
 
@@ -364,8 +365,9 @@ static void prepare_save_tm_user_regs(void)
 		current->thread.ckvrsave = mfspr(SPRN_VRSAVE);
 }
 
-static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
-				    struct mcontext __user *tm_frame, unsigned long msr)
+static __always_inline int
+save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
+			 struct mcontext __user *tm_frame, unsigned long msr)
 {
 	/* Save both sets of general registers */
 	unsafe_save_general_regs(&current->thread.ckpt_regs, frame, failed);
@@ -444,8 +446,9 @@ static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user
 #else
 static void prepare_save_tm_user_regs(void) { }
 
-static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
-				    struct mcontext __user *tm_frame, unsigned long msr)
+static __always_inline int
+save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
+			 struct mcontext __user *tm_frame, unsigned long msr)
 {
 	return 0;
 }
-- 
2.40.1

On Mon, 05 Jun 2023 10:58:35 +0200, Christophe Leroy wrote:
> Looking at generated code for handle_signal32() shows calls to a
> function called __unsafe_save_user_regs.constprop.0 while user access
> is open.
> 
> And that __unsafe_save_user_regs.constprop.0 function has two nops at
> the begining, allowing it to be traced, which is unexpected during
> user access open window.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/signal32: Force inlining of __unsafe_save_user_regs() and save_tm_user_regs_unsafe()
      https://git.kernel.org/powerpc/c/a03b1a0b19398a47489fdcef02ec19c2ba05a15d

cheers

On Mon Jun 5, 2023 at 6:58 PM AEST, Christophe Leroy wrote:
> Looking at generated code for handle_signal32() shows calls to a
> function called __unsafe_save_user_regs.constprop.0 while user access
> is open.
>
> And that __unsafe_save_user_regs.constprop.0 function has two nops at
> the begining, allowing it to be traced, which is unexpected during
> user access open window.
>
> The solution could be to mark __unsafe_save_user_regs() no trace, but
> to be on the safe side the most efficient is to flag it __always_inline
> as already done for function __unsafe_restore_general_regs(). The
> function is relatively small and only called twice, so the size
> increase will remain in the noise.
>
> Do the same with save_tm_user_regs_unsafe() as it may suffer the
> same issue.

Could you put a comment so someone doesn't uninline it later? Marking
it notrace as well would be sufficient for a comment, if that works.

Thanks,
Nick

>
> Fixes: ef75e7318294 ("powerpc/signal32: Transform save_user_regs() and save_tm_user_regs() in 'unsafe' version")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/signal_32.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index c114c7f25645..7a718ed32b27 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -264,8 +264,9 @@ static void prepare_save_user_regs(int ctx_has_vsx_region)
>  #endif
>  }
>  
> -static int __unsafe_save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
> -				   struct mcontext __user *tm_frame, int ctx_has_vsx_region)
> +static __always_inline int
> +__unsafe_save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
> +			struct mcontext __user *tm_frame, int ctx_has_vsx_region)
>  {
>  	unsigned long msr = regs->msr;
>  
> @@ -364,8 +365,9 @@ static void prepare_save_tm_user_regs(void)
>  		current->thread.ckvrsave = mfspr(SPRN_VRSAVE);
>  }
>  
> -static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
> -				    struct mcontext __user *tm_frame, unsigned long msr)
> +static __always_inline int
> +save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
> +			 struct mcontext __user *tm_frame, unsigned long msr)
>  {
>  	/* Save both sets of general registers */
>  	unsafe_save_general_regs(&current->thread.ckpt_regs, frame, failed);
> @@ -444,8 +446,9 @@ static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user
>  #else
>  static void prepare_save_tm_user_regs(void) { }
>  
> -static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
> -				    struct mcontext __user *tm_frame, unsigned long msr)
> +static __always_inline int
> +save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
> +			 struct mcontext __user *tm_frame, unsigned long msr)
>  {
>  	return 0;
>  }
> -- 
> 2.40.1

"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Mon Jun 5, 2023 at 6:58 PM AEST, Christophe Leroy wrote:
>> Looking at generated code for handle_signal32() shows calls to a
>> function called __unsafe_save_user_regs.constprop.0 while user access
>> is open.
>>
>> And that __unsafe_save_user_regs.constprop.0 function has two nops at
>> the begining, allowing it to be traced, which is unexpected during
>> user access open window.
>>
>> The solution could be to mark __unsafe_save_user_regs() no trace, but
>> to be on the safe side the most efficient is to flag it __always_inline
>> as already done for function __unsafe_restore_general_regs(). The
>> function is relatively small and only called twice, so the size
>> increase will remain in the noise.
>>
>> Do the same with save_tm_user_regs_unsafe() as it may suffer the
>> same issue.
>
> Could you put a comment so someone doesn't uninline it later?

I think the "unsafe" in the name is probably sufficient to warn people
off, but you never know. Still I'd happily take a patch to add comments :)

> Marking it notrace as well would be sufficient for a comment, if that works.

I nearly did that when applying, but I'm not sure it won't change the
code generation, so I left it as-is.

cheers