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

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

在 2024/8/2 17:24, Oleg Nesterov 写道:
> On 08/02, Liao, Chang wrote:
>>
>>
>> 在 2024/8/1 22:06, Oleg Nesterov 写道:
>>> On 08/01, Liao Chang wrote:
>>>>
>>>> @@ -2276,22 +2277,25 @@ 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
>>>> +		fallthrough;
>>>> +	case UTASK_SSTEP_DENY_SIGNAL:
>>>> +		set_tsk_thread_flag(current, TIF_SIGPENDING);
>>>> +		break;
>>>> +	default:
>>>>  		WARN_ON_ONCE(1);
>>>> +	}
>>>
>>> Liao, at first glance this change looks "obviously wrong" to me.
>>
>> Oleg. Did i overlook some thing obvious here?
> 
> OK, lets suppose uprobe_deny_signal() sets UTASK_SSTEP_DENY_SIGNAL.
> 
> In this case handle_singlestep() will only set TIF_SIGPENDING and
> do nothing else. This is wrong, either _post_xol() or _abort_xol()
> must be called.
> 
> But I think handle_singlestep() will never hit this case. In the
> likely case uprobe_post_sstep_notifier() will replace _DENY_SIGNAL
> with _ACK, and this means that handle_singlestep() won't restore
> TIF_SIGPENDING cleared by uprobe_deny_signal().

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:

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