On Tue, Jun 23, 2026 at 9:25 AM Oleg Nesterov <[email protected]> wrote: > > On 06/23, Peter Zijlstra wrote: > > > > On Tue, Jun 23, 2026 at 02:52:32PM +0200, Oleg Nesterov wrote: > > > On 06/22, David Windsor wrote: > > > > > > > > --- a/arch/x86/kernel/uprobes.c > > > > +++ b/arch/x86/kernel/uprobes.c > > > > @@ -1246,8 +1246,12 @@ static int default_post_xol_op(struct > > > > arch_uprobe *auprobe, struct pt_regs *regs > > > > long correction = utask->vaddr - utask->xol_vaddr; > > > > regs->ip += correction; > > > > } else if (auprobe->defparam.fixups & UPROBE_FIX_CALL) { > > > > + unsigned long retaddr = utask->vaddr + auprobe->defparam.ilen; > > > > + > > > > regs->sp += sizeof_long(regs); /* Pop incorrect return > > > > address */ > > > > - if (emulate_push_stack(regs, utask->vaddr + > > > > auprobe->defparam.ilen)) > > > > + if (emulate_push_stack(regs, retaddr)) > > > > + return -ERESTART; > > > > + if (shstk_update_last_frame(retaddr)) > > > > return -ERESTART; > > > > > > Well, if shstk_update_last_frame() fails after emulate_push_stack(), we > > > should > > > probably return another error, so that the caller handle_singlestep() > > > will kill > > > this task? > > > > Makes sense, the other user has a force_sig(SIGSEGV) on failure. > > Offtopic question... both shstk_update_last_frame() and shstk_push() are only > used by arch/x86/kernel/uprobes.c. But they are not symmetric in that > shstk_update_last_frame() returns 0 if !features_enabled(ARCH_SHSTK_SHSTK), > while shstk_push() returns -ENOTSUPP in this case. > > That is why the users can't just do "if (shstk_push(xxx)) ...". This is really > minor, but perhaps it makes sense to change shstk_push() to return 0 in this > case too? I don't think -ENOTSUPP is actually useful... >
Agreed, will send a follow-up patch changing shstk_push() to return 0 rather than -ENOTSUPP. I'll send v2 shortly with the additional call to force_sig(SIGSEGV) to balance out the callers. > Oleg. >
