linux-kernel - Re: [PATCH v5 1/2] uprobes: Remove redundant spinlock in uprobe_deny

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250124172435.GB13891@redhat.com>
Date: Fri, 24 Jan 2025 18:25:36 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Liao Chang <liaochang1@...wei.com>, mhiramat@...nel.org,
	peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
	namhyung@...nel.org, mark.rutland@....com,
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, andrii.nakryiko@...il.com,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
	linux-perf-users@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH v5 1/2] uprobes: Remove redundant spinlock in
 uprobe_deny_signal()

On 01/24, Steven Rostedt wrote:
>
> On Fri, 24 Jan 2025 09:38:25 +0000
> Liao Chang <liaochang1@...wei.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.