| Message ID | 1682213883-3654-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 46e614cc91f7bd6f7872e434e6bcfda44454aac3 |
| Headers | show |
| Series | MIPS: uprobes: Restore thread.trap_nr | expand |
On Sun, Apr 23, 2023 at 09:38:03AM +0800, Tiezhu Yang wrote: > thread.trap_nr is saved in arch_uprobe_pre_xol(), it should be restored > in arch_uprobe_{post,abort}_xol() accordingly, actually it was only done > in the post function, just do it in the abort function too, this change > is similar with x86 and powerpc. I'm confused (please fix up grammar, spelling, and punctuation). Can you explain why thread.trap_nr should be restored somewhere else? Also, what x86/powerpc changes as reference? Thanks.
Cc: Oleg Nesterov <oleg@redhat.com> Srikar Dronamraju <srikar@linux.vnet.ibm.com> On 04/23/2023 11:08 AM, Bagas Sanjaya wrote: > On Sun, Apr 23, 2023 at 09:38:03AM +0800, Tiezhu Yang wrote: >> thread.trap_nr is saved in arch_uprobe_pre_xol(), it should be restored >> in arch_uprobe_{post,abort}_xol() accordingly, actually it was only done >> in the post function, just do it in the abort function too, this change >> is similar with x86 and powerpc. > > I'm confused (please fix up grammar, spelling, and punctuation). Can you > explain why thread.trap_nr should be restored somewhere else? Also, what > x86/powerpc changes as reference? > Here is the related first commit for x86 in 2012: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0326f5a94dde When xol insn itself triggers the signal, restart the original insn, in this case, UTASK_SSTEP_TRAPPED is set [1], it does *abort_xol() instead of *post_xol() [2], then should do the restore operations. Maybe Oleg and Srikar could give more detailed backgrounds, thank you. https://lore.kernel.org/lkml/1682213883-3654-1-git-send-email-yangtiezhu@loongson.cn/ [1] https://elixir.bootlin.com/linux/latest/source/kernel/events/uprobes.c#L1980 [2] https://elixir.bootlin.com/linux/latest/source/kernel/events/uprobes.c#L2268 Thanks, Tiezhu
On 04/23, Tiezhu Yang wrote: > > Cc: > Oleg Nesterov <oleg@redhat.com> > Srikar Dronamraju <srikar@linux.vnet.ibm.com> > > On 04/23/2023 11:08 AM, Bagas Sanjaya wrote: > >On Sun, Apr 23, 2023 at 09:38:03AM +0800, Tiezhu Yang wrote: > >>thread.trap_nr is saved in arch_uprobe_pre_xol(), it should be restored > >>in arch_uprobe_{post,abort}_xol() accordingly, actually it was only done > >>in the post function, just do it in the abort function too, this change > >>is similar with x86 and powerpc. > > > >I'm confused (please fix up grammar, spelling, and punctuation). Can you > >explain why thread.trap_nr should be restored somewhere else? Also, what > >x86/powerpc changes as reference? > > > > Here is the related first commit for x86 in 2012: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0326f5a94dde > > When xol insn itself triggers the signal, restart the original insn, > in this case, UTASK_SSTEP_TRAPPED is set [1], it does *abort_xol() > instead of *post_xol() [2], then should do the restore operations. Yes... for example, if the uprobed task was killed abort() should restore the state and (in particular) change ->trap_nr from UPROBE_TRAP_NR back to ->saved_trap_nr. So the patch looks fine to me. Oleg.
On Sun, Apr 23, 2023 at 09:38:03AM +0800, Tiezhu Yang wrote: > thread.trap_nr is saved in arch_uprobe_pre_xol(), it should be restored > in arch_uprobe_{post,abort}_xol() accordingly, actually it was only done > in the post function, just do it in the abort function too, this change > is similar with x86 and powerpc. > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > arch/mips/kernel/uprobes.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/mips/kernel/uprobes.c b/arch/mips/kernel/uprobes.c > index 6b630ed..401b148 100644 > --- a/arch/mips/kernel/uprobes.c > +++ b/arch/mips/kernel/uprobes.c > @@ -191,6 +191,7 @@ void arch_uprobe_abort_xol(struct arch_uprobe *aup, > { > struct uprobe_task *utask = current->utask; > > + current->thread.trap_nr = utask->autask.saved_trap_nr; > instruction_pointer_set(regs, utask->vaddr); > } > > -- > 2.1.0 applied to mips-next. Thomas.
diff --git a/arch/mips/kernel/uprobes.c b/arch/mips/kernel/uprobes.c index 6b630ed..401b148 100644 --- a/arch/mips/kernel/uprobes.c +++ b/arch/mips/kernel/uprobes.c @@ -191,6 +191,7 @@ void arch_uprobe_abort_xol(struct arch_uprobe *aup, { struct uprobe_task *utask = current->utask; + current->thread.trap_nr = utask->autask.saved_trap_nr; instruction_pointer_set(regs, utask->vaddr); }
thread.trap_nr is saved in arch_uprobe_pre_xol(), it should be restored in arch_uprobe_{post,abort}_xol() accordingly, actually it was only done in the post function, just do it in the abort function too, this change is similar with x86 and powerpc. Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- arch/mips/kernel/uprobes.c | 1 + 1 file changed, 1 insertion(+)