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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250124102702.6ff0ccc5@gandalf.local.home>
Date: Fri, 24 Jan 2025 10:27:02 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Liao Chang <liaochang1@...wei.com>
Cc: <mhiramat@...nel.org>, <oleg@...hat.com>, <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 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.

There may be another path that tests the flag within a spin lock, and then
does a bunch of work assuming that the flag does not change while it is
doing that work. That other path would require a spin lock around the
clearing of the flag elsewhere.

I don't know this code well enough to know if this has that scenario, and
seeing the Acked-by from Oleg, I'm assuming it does not. But in any case,
the change log needs to give a better rationale for removing a spin lock than
just "clearing a flag atomically doesn't need a spin lock"!

-- Steve

> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> Acked-by: Oleg Nesterov <oleg@...hat.com>
> Signed-off-by: Liao Chang <liaochang1@...wei.com>