Re: [PATCH v5 1/2] uprobes: Remove redundant spinlock in uprobe_deny_signal()

[Oleg Nesterov <oleg@redhat.com>]

On 01/24, Steven Rostedt wrote:
>
> On Fri, 24 Jan 2025 09:38:25 +0000
> Liao Chang <liaochang1@huawei.com> wrote:
>
> > Since clearing a bit in thread_info is an atomic operation, the spinlock
> > is redundant and can be removed, reducing lock contention is good for
> > performance.
>
> Although this patch is probably fine, the change log suggests a dangerous
> precedence. Just because clearing a flag is atomic, that alone does not
> guarantee that it doesn't need spin locks around it.

Yes. And iirc we already have the lockless users of clear(TIF_SIGPENDING)
(some if not most of them look buggy). But afaics in this (very special)
case it should be fine.

See also https://lore.kernel.org/all/20240812120738.GC11656@redhat.com/

> There may be another path that tests the flag within a spin lock,

Yes, retarget_shared_pending() or the complete_signal/wants_signal loop.
That is why it was decided to take siglock in uprobe_deny_signal(), just
to be "safe".

But I still think this patch is fine. The current task is going to execute
a single insn which can't enter the kernel and/or return to the userspace
before it calls handle_singlestep() and restores TIF_SIGPENDING. We do not
care if it races with another source of TIF_SIGPENDING.

The only problem is that task_sigpending() from another task can "wrongly"
return false in this window, but I don't see any problem.

Oleg.