在 2024/8/7 18:17, Oleg Nesterov 写道:
> 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, If i understand correctly, current state machine expects the single-step
handling
should end up with either _ACK or _TRAPPED. Any new state would disrupt this
logic. If so,
I'm convinced that adding a new state is uncessary. As you mentioned, I propose
using a
boolean flag in the uprobe_task data to track whether a signal should be
restored at the
cost of increased size. Here's outline of the changes:
- pre_ssout() resets the deny signal flag
- uprobe_deny_signal() sets the deny signal flag when TIF_SIGPENDING is
cleared.
- handle_singlestep() check the deny signal flag and restore TIF_SIGPENDING
if necessary.
Does this approach look correct to you,do do you have any other way to
implement the "flag"?
Thanks.
>
> 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
>>>
>
>
--
BR
Liao, Chang
