Re: [PATCH] uprobes: Improve scalability by reducing the contention on siglock

["Liao, Chang" <liaochang1@huawei.com>]

在 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(&current->sighand->siglock);
>>> -       recalc_sigpending(); /* see uprobe_deny_signal() */
>>> -       spin_unlock_irq(&current->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