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
> >
