So. Liao, I am sorry, but I dislike your patch/approach in any case. UTASK_SSTEP_DENY_SIGNAL complicates the state machine. And I don't like the fact that set_thread_flag(TIF_SIGPENDING) is called twice, from handle_singlestep() and uprobe_post_sstep_notifier(), this complicates the logic even more.
We need a flag, not the new state. And if I read this patch correctly it is wrong: - uprobe_deny_signal() clears TIF_SIGPENDING and sets UTASK_SSTEP_DENY_SIGNAL - another signal cames after that and sets TIF_SIGPENDING again - in this case the task won't return to user-space and execute the probed insn, exit_to_user_mode_loop() will notice another TIF_SIGPENDING and call arch_do_signal_or_restart()->get_signal() again. - get_signal() will call uprobe_deny_signal() again hit WARN_ON_ONCE(utask->state != UTASK_SSTEP); And no, we shouldn't change this check into UTASK_SSTEP || UTASK_SSTEP_DENY_SIGNAL. Again, the fact that uprobe_deny_signal() cleared TIF_SIGPENDING must not be the new state. Oleg. On 08/06, Oleg Nesterov wrote: > > On 08/06, Liao, Chang wrote: > > > > You're absolutely right. handle_signlestep() has chance to handle > > _DENY_SIGANL > > unless it followed by setting TIF_UPROBE in uprobe_deny_signal(). This means > > _DENY_SIGNAL is likey replaced during next uprobe single-stepping. > > > > I believe introducing _DENY_SIGNAL as the immediate state between > > UTASK_SSTEP > > and UTASK_SSTEP_ACK is still necessary. This allow > > uprobe_post_sstep_notifier() > > to correctly restore TIF_SIGPENDING upon the completion of single-step. > > > > A revised implementation would look like this: > > Still looks "obviously wrong" to me... even the approach itself. > > Perhaps I am wrong, yet another day when I can't even read emails on lkml > carefully, sorry. > > But can you please send the patch which I could actually apply? This one > looks white-space damaged... > > I'll try to reply with more details as soon I convince myself I fully > understand what does your patch actually do, but most probably not tomorrow. > > Thanks, > > Oleg. > > > ------------------%<------------------ > > --- a/kernel/events/uprobes.c > > +++ b/kernel/events/uprobes.c > > @@ -1980,6 +1980,7 @@ bool uprobe_deny_signal(void) > > > > if (task_sigpending(t)) { > > clear_tsk_thread_flag(t, TIF_SIGPENDING); > > + utask->state = UTASK_SSTEP_DENY_SIGNAL; > > > > if (__fatal_signal_pending(t) || > > arch_uprobe_xol_was_trapped(t)) { > > utask->state = UTASK_SSTEP_TRAPPED; > > @@ -2276,22 +2277,23 @@ static void handle_singlestep(struct uprobe_task > > *utask, struct pt_regs *regs) > > int err = 0; > > > > uprobe = utask->active_uprobe; > > - if (utask->state == UTASK_SSTEP_ACK) > > + switch (utask->state) { > > + case UTASK_SSTEP_ACK: > > err = arch_uprobe_post_xol(&uprobe->arch, regs); > > - else if (utask->state == UTASK_SSTEP_TRAPPED) > > + break; > > + case UTASK_SSTEP_TRAPPED: > > arch_uprobe_abort_xol(&uprobe->arch, regs); > > - else > > + set_thread_flag(TIF_SIGPENDING); > > + break; > > + default: > > WARN_ON_ONCE(1); > > + } > > > > put_uprobe(uprobe); > > utask->active_uprobe = NULL; > > utask->state = UTASK_RUNNING; > > xol_free_insn_slot(current); > > > > - spin_lock_irq(¤t->sighand->siglock); > > - recalc_sigpending(); /* see uprobe_deny_signal() */ > > - spin_unlock_irq(¤t->sighand->siglock); > > - > > if (unlikely(err)) { > > uprobe_warn(current, "execute the probed insn, sending > > SIGILL."); > > force_sig(SIGILL); > > @@ -2351,6 +2353,8 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs) > > /* task is currently not uprobed */ > > return 0; > > > > + if (utask->state == UTASK_SSTEP_DENY_SIGNAL) > > + set_thread_flag(TIF_SIGPENDING); > > utask->state = UTASK_SSTEP_ACK; > > set_thread_flag(TIF_UPROBE); > > return 1; > > > > ------------------>%------------------ > > > > > > > > Oleg. > > > > > > > > > > -- > > BR > > Liao, Chang > >