Message ID | 7e469c8f01860a69c1ada3ca6a5e2aa65f0f74b2.1685955220.git.christophe.leroy@csgroup.eu (mailing list archive) |
---|---|
State | Accepted |
Commit | a03b1a0b19398a47489fdcef02ec19c2ba05a15d |
Headers | show |
Series | powerpc/signal32: Force inlining of __unsafe_save_user_regs() and save_tm_user_regs_unsafe() | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
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(¤t->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
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
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(¤t->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; }
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(-)