On Thu, Oct 31, 2024 at 1:45 AM Kevin Brodsky <kevin.brodsky@xxxxxxx> wrote: > > On 30/10/2024 23:01, Jeff Xu wrote: > >> -static int restore_poe_context(struct user_ctxs *user) > >> +static int restore_poe_context(struct user_ctxs *user, > >> + struct user_access_state *ua_state) > >> { > >> u64 por_el0; > >> int err = 0; > >> @@ -282,7 +338,7 @@ static int restore_poe_context(struct user_ctxs *user) > >> > >> __get_user_error(por_el0, &(user->poe->por_el0), err); > >> if (!err) > >> - write_sysreg_s(por_el0, SYS_POR_EL0); > >> + ua_state->por_el0 = por_el0; > >> > >> return err; > >> } > >> @@ -850,7 +906,8 @@ static int parse_user_sigframe(struct user_ctxs *user, > >> } > >> > >> static int restore_sigframe(struct pt_regs *regs, > >> - struct rt_sigframe __user *sf) > >> + struct rt_sigframe __user *sf, > >> + struct user_access_state *ua_state) > >> { > >> sigset_t set; > >> int i, err; > >> @@ -899,7 +956,7 @@ static int restore_sigframe(struct pt_regs *regs, > >> err = restore_zt_context(&user); > >> > >> if (err == 0 && system_supports_poe() && user.poe) > >> - err = restore_poe_context(&user); > >> + err = restore_poe_context(&user, ua_state); > >> > >> return err; > >> } > >> @@ -908,6 +965,7 @@ SYSCALL_DEFINE0(rt_sigreturn) > >> { > >> struct pt_regs *regs = current_pt_regs(); > >> struct rt_sigframe __user *frame; > >> + struct user_access_state ua_state; > >> > >> /* Always make any pending restarted system calls return -EINTR */ > >> current->restart_block.fn = do_no_restart_syscall; > >> @@ -924,12 +982,14 @@ SYSCALL_DEFINE0(rt_sigreturn) > >> if (!access_ok(frame, sizeof (*frame))) > >> goto badframe; > >> > >> - if (restore_sigframe(regs, frame)) > >> + if (restore_sigframe(regs, frame, &ua_state)) > >> goto badframe; > >> > >> if (restore_altstack(&frame->uc.uc_stack)) > >> goto badframe; > >> > > Do you need to move restore_altstack ahead of restore_sigframe? > > This is not necessary because restore_sigframe() no longer writes to > POR_EL0. restore_poe_context() (above) now saves the original POR_EL0 > value into ua_state, and it is restore_user_access_state() (called below > just before returning to userspace) that actually writes to POR_EL0, > after all uaccess is completed. > Got it, thanks for the explanation. -Jeff > Having said that, I somehow missed the call to restore_altstack() when > writing the commit message, so these changes in sys_rt_sigreturn are in > fact necessary. Good catch! At least the patch itself should be doing > the right thing. > > - Kevin > > > similar as x86 change [1], > > the discussion for this happened in [2] [3] > > > > [1] https://lore.kernel.org/lkml/20240802061318.2140081-5-aruna.ramakrishna@xxxxxxxxxx/ > > [2] https://lore.kernel.org/lkml/20240425210540.3265342-1-jeffxu@xxxxxxxxxxxx/ > > [3] https://lore.kernel.org/lkml/d0162c76c25bc8e1c876aebe8e243ff2e6862359.camel@xxxxxxxxx/ > > > > Thanks > > -Jeff > > > > > >> + restore_user_access_state(&ua_state); > >> + > >> return regs->regs[0]; >