[PATCH bpf-next 1/2] bpf: Only check nmi_uaccess_okay() when signalling current

[PATCH bpf-next 1/2] bpf: Only check nmi_uaccess_okay() when signalling current

[Aditya Sharma]

nmi_uaccess_okay() takes no task argument and is a statement about
current. When commit 6280cf718db0 ("bpf: Implement bpf_send_signal_task()
kfunc") made the PF_KTHREAD/PF_EXITING and is_global_init() test the
supplied task, nmi_uaccess_okay() was left testing current.

As a result bpf_send_signal_task() returns -EPERM whenever the
calling context happens to be a kernel thread, regardless of which task
the signal is aimed at. The same call with the same target succeeds or
fails depending only on what the CPU was running:

  bpf_send_signal_task() from tp_btf/workqueue_execute_start : -EPERM
  bpf_send_signal_task() from tp_btf/sys_enter               : 0

This was originally hit from a bpf_timer callback, where the failure
is intermittent because a softirq runs on whichever task it
interrupted. The rejection is x86-only, as nmi_uaccess_okay() is
defined as true in include/asm-generic/tlb.h elsewhere.

Only apply the check when the signal is sent to current, where the
predicate is meaningful. bpf_send_signal() and bpf_send_signal_thread()
assign task = current, so their behaviour is unchanged.

Fixes: 6280cf718db0 ("bpf: Implement bpf_send_signal_task() kfunc")
Suggested-by: Yonghong Song <yonghong.song@linux.dev>
Link: https://lore.kernel.org/bpf/20260819124324.43162-1-adi.sharma@zohomail.in/T/#u

Signed-off-by: Aditya Sharma <adi.sharma@zohomail.in>
---
 kernel/trace/bpf_trace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 29260951aa87..f7a41f222599 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -871,7 +871,10 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struc
 	 */
 	if (unlikely(task->flags & (PF_KTHREAD | PF_EXITING)))
 		return -EPERM;
-	if (unlikely(!nmi_uaccess_okay()))
+	/* Since nmi_uaccess_okay() is only for the current
+	 * task, check if task is current.
+	 */
+	if (task == current && unlikely(!nmi_uaccess_okay()))
 		return -EPERM;
 	/* Task should not be pid=1 to avoid kernel panic. */
 	if (unlikely(is_global_init(task)))

base-commit: d761934c9483ecde93fe99d8705282f716dfee50
-- 
2.34.1

[PATCH bpf-next 2/2] selftests/bpf: Test bpf_send_signal_task() from kernel thread context

[Aditya Sharma]

The existing send_signal tests, including the _remote variants that
exercise bpf_send_signal_task() with a caller-supplied target, all
gate on

  if ((bpf_get_current_pid_tgid() >> 32) == pid)

so the BPF program only ever runs while current is the test process.
The caller-context dimension is therefore untested, which is how
bpf_send_signal_task() failing from kernel thread context went
unnoticed.

Add a program attached to tp_btf/workqueue_execute_start, where
current is always a kworker. It cannot filter on current, so it is
driven by target_pid alone, and the test asserts the child receives
the signal. Setting pid to 0 leaves the pre-existing programs in the
skeleton inert. Workqueues run on their own, so nothing has to be
triggered from userspace.

Without narrowing down the nmi_uaccess_okay() check to task == current,
this test fails on x86.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Aditya Sharma <adi.sharma@zohomail.in>
---
 .../selftests/bpf/prog_tests/send_signal.c    | 104 ++++++++++++++++++
 .../bpf/progs/test_send_signal_kern.c         |  26 +++++
 2 files changed, 130 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 7ac4d5a488aa..f9dd23db1ef9 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -261,6 +261,108 @@ static void test_send_signal_nmi(bool signal_thread, bool remote)
 	test_send_signal_common(&attr, signal_thread, remote);
 }
 
+static void test_send_signal_kworker(void)
+{
+	struct test_send_signal_kern *skel;
+	int pipe_c2p[2], pipe_p2c[2];
+	struct sigaction sa = {};
+	char buf[256];
+	int err = -1;
+	pid_t pid;
+
+	if (!ASSERT_OK(pipe(pipe_c2p), "pipe_c2p"))
+		return;
+
+	if (!ASSERT_OK(pipe(pipe_p2c), "pipe_p2c")) {
+		close(pipe_c2p[0]);
+		close(pipe_c2p[1]);
+		return;
+	}
+
+	pid = fork();
+	if (!ASSERT_GE(pid, 0, "fork")) {
+		close(pipe_c2p[0]);
+		close(pipe_c2p[1]);
+		close(pipe_p2c[0]);
+		close(pipe_p2c[1]);
+		return;
+	}
+
+	if (pid == 0) {
+		/* install signal handler and notify parent */
+		sa.sa_sigaction = sigusr1_siginfo_handler;
+		sa.sa_flags = SA_RESTART | SA_SIGINFO;
+		ASSERT_NEQ(sigaction(SIGUSR1, &sa, NULL), -1, "sigaction");
+
+		close(pipe_c2p[0]); /* close read */
+		close(pipe_p2c[1]); /* close write */
+
+		/* notify parent signal handler is installed */
+		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
+
+		/* make sure parent enabled bpf program to send_signal */
+		ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
+
+		/* the signal is sent from workqueue context, so nothing has
+		 * to be triggered from here
+		 */
+		while (!sigusr1_received)
+			sleep(1);
+
+		buf[0] = sigusr1_received;
+
+		ASSERT_EQ(sigusr1_received, 8, "sigusr1_received");
+		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
+
+		close(pipe_c2p[1]);
+		close(pipe_p2c[0]);
+		exit(0);
+	}
+
+	close(pipe_c2p[1]); /* close write */
+	close(pipe_p2c[0]); /* close read */
+
+	skel = test_send_signal_kern__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
+		goto skel_open_load_failure;
+
+	/* wait until child signal handler installed */
+	ASSERT_EQ(read(pipe_c2p[0], buf, 1), 1, "pipe_read");
+
+	/* pid == 0 keeps the other programs of this skeleton inactive */
+	skel->bss->pid = 0;
+	skel->bss->sig = SIGUSR1;
+	skel->bss->target_pid = pid;
+
+	err = test_send_signal_kern__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach")) {
+		err = -1;
+		goto destroy_skel;
+	}
+
+	/* notify child that bpf program can send_signal now */
+	ASSERT_EQ(write(pipe_p2c[1], buf, 1), 1, "pipe_write");
+
+	/* wait for result, workqueues run on their own */
+	err = read_with_timeout(pipe_c2p[0], buf, 1, 10 * 1000 * 1000);
+	if (!ASSERT_GT(err, 0, "reading pipe"))
+		goto destroy_skel;
+
+	ASSERT_EQ(buf[0], 8, "incorrect result");
+
+destroy_skel:
+	test_send_signal_kern__destroy(skel);
+skel_open_load_failure:
+	close(pipe_c2p[0]);
+	close(pipe_p2c[1]);
+	/*
+	 * Child is either about to exit cleanly or stuck in case of errors.
+	 * Nudge it to exit.
+	 */
+	kill(pid, SIGKILL);
+	wait(NULL);
+}
+
 void test_send_signal(void)
 {
 	if (test__start_subtest("send_signal_tracepoint"))
@@ -289,4 +391,6 @@ void test_send_signal(void)
 		test_send_signal_perf(true, true);
 	if (test__start_subtest("send_signal_nmi_thread_remote"))
 		test_send_signal_nmi(true, true);
+	if (test__start_subtest("send_signal_kworker"))
+		test_send_signal_kworker();
 }
diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
index 176a355e3062..d1c000c30885 100644
--- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
@@ -60,6 +60,32 @@ int send_signal_tp_sched(void *ctx)
 	return bpf_send_signal_test(ctx);
 }
 
+/* Send a signal to a task other than current, from a context where current
+ * is a kernel thread. No filtering on current is possible here, so this is
+ * driven entirely by target_pid.
+ */
+SEC("tp_btf/workqueue_execute_start")
+int send_signal_kworker(void *ctx)
+{
+	struct task_struct *target_task;
+	int ret;
+
+	if (status != 0 || target_pid == 0)
+		return 0;
+
+	target_task = bpf_task_from_pid(target_pid);
+	if (!target_task)
+		return 0;
+
+	ret = bpf_send_signal_task(target_task, sig, PIDTYPE_TGID, 8);
+	bpf_task_release(target_task);
+
+	if (ret == 0)
+		status = 1;
+
+	return 0;
+}
+
 SEC("perf_event")
 int send_signal_perf(void *ctx)
 {
-- 
2.34.1

Re: [PATCH bpf-next 1/2] bpf: Only check nmi_uaccess_okay() when signalling current

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Multi-line comment does not follow BPF subsystem standards.

Pre-existing issues:
- [High] A TOCTOU data race on the per-CPU `send_signal_work` structure allows NMI reentrancy to corrupt irq_work data and leak a task_struct reference.
--

commit b0fb28992ab88d35a628c6d9650cd4ab7e67c397
Author: Aditya Sharma <adi.sharma@zohomail.in>

bpf: Only check nmi_uaccess_okay() when signalling current

nmi_uaccess_okay() takes no task argument and is a statement about current.
This commit restricts the check to only apply when signalling the current
task, fixing a bug where bpf_send_signal_task() returned -EPERM for kernel
threads.

Link: https://lore.kernel.org/bpf/20260819124324.43162-1-adi.sharma@zohomail.in/T/#u

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 29260951aa871..f7a41f2225999 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -871,7 +871,10 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struc
>  	 */
>  	if (unlikely(task->flags & (PF_KTHREAD | PF_EXITING)))
>  		return -EPERM;
> -	if (unlikely(!nmi_uaccess_okay()))
> +	/* Since nmi_uaccess_okay() is only for the current
> +	 * task, check if task is current.
> +	 */

[Severity: Low]
This isn't a bug, but does this newly added comment violate the BPF subsystem
multi-line comment style?

The BPF subsystem strictly requires multi-line comments to have the opening
/* on its own line, with the text starting on the next line.

> +	if (task == current && unlikely(!nmi_uaccess_okay()))
>  		return -EPERM;
>  	/* Task should not be pid=1 to avoid kernel panic. */
>  	if (unlikely(is_global_init(task)))

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU data race on the per-CPU
send_signal_work structure later in bpf_send_signal_common()?

If a BPF program executing in a context with IRQs disabled calls
bpf_send_signal(), it evaluates irq_work_is_busy() as false. Before it
queues the work, an NMI could fire on the same CPU and also call
bpf_send_signal():

kernel/trace/bpf_trace.c:bpf_send_signal_common() {
        ...
	if (preempt_count() != 0 || irqs_disabled()) {
                ...
		work = this_cpu_ptr(&send_signal_work);
		if (irq_work_is_busy(&work->irq_work))
			return -EBUSY;

                /* NMI fires here, calls bpf_send_signal_common(), evaluating
                 * irq_work_is_busy() as false since it's not queued yet.
                 * NMI writes its data and queues the work.
                 */

		work->task = get_task_struct(task);
		work->has_siginfo = siginfo == &info;
		if (work->has_siginfo)
			copy_siginfo(&work->info, &info);
		work->sig = sig;
		work->type = type;
		irq_work_queue(&work->irq_work);
		return 0;
	}
        ...
}

Upon return from the NMI, the interrupted context resumes, overwrites
work->task with a new get_task_struct(), and fails to queue the work since
the NMI already queued it.

Could this allow NMI reentrancy to corrupt irq_work data and permanently leak
a task_struct reference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902150439.692725-1-adi.sharma@zohomail.in?part=1

Re: [PATCH bpf-next 2/2] selftests/bpf: Test bpf_send_signal_task() from kernel thread context

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly added `send_signal_kworker` program silently runs during existing remote tests, sending the signal instead of the intended programs (like perf or NMI) and invalidating their test coverage.
- [Low] Multi-line comments do not follow the BPF subsystem style guidelines.
--

commit 2bcfd3bbad998237f70d4a407001c43f11ecd4c9
Author: Aditya Sharma <adi.sharma@zohomail.in>
selftests/bpf: Test bpf_send_signal_task() from kernel thread context

This commit adds a new test to verify bpf_send_signal_task() from a kernel
thread context. It attaches a BPF program to a workqueue tracepoint to
ensure signals can be properly sent when the caller is a kworker.

> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 7ac4d5a488aa5..f9dd23db1ef98 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
[ ... ]
> +	if (pid == 0) {
> +		/* install signal handler and notify parent */
> +		sa.sa_sigaction = sigusr1_siginfo_handler;
> +		sa.sa_flags = SA_RESTART | SA_SIGINFO;
> +		ASSERT_NEQ(sigaction(SIGUSR1, &sa, NULL), -1, "sigaction");
> +
> +		close(pipe_c2p[0]); /* close read */
> +		close(pipe_p2c[1]); /* close write */
> +
> +		/* notify parent signal handler is installed */
> +		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
> +
> +		/* make sure parent enabled bpf program to send_signal */
> +		ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
> +
> +		/* the signal is sent from workqueue context, so nothing has
> +		 * to be triggered from here
> +		 */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the BPF subsystem
style guidelines? The BPF subsystem mandates that multi-line comments must
have the opening /* on its own line.

> +		while (!sigusr1_received)
> +			sleep(1);
> +
> +		buf[0] = sigusr1_received;
> +
> +		ASSERT_EQ(sigusr1_received, 8, "sigusr1_received");
> +		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
> index 176a355e30624..d1c000c308857 100644
> --- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
[ ... ]
> @@ -60,6 +60,32 @@ int send_signal_tp_sched(void *ctx)
>  	return bpf_send_signal_test(ctx);
>  }
>  
> +/* Send a signal to a task other than current, from a context where current
> + * is a kernel thread. No filtering on current is possible here, so this is
> + * driven entirely by target_pid.
> + */

[Severity: Low]
This isn't a bug, but similar to the comment in send_signal.c, should the
opening /* be on its own line here as well to match the BPF style
guidelines?

> +SEC("tp_btf/workqueue_execute_start")
> +int send_signal_kworker(void *ctx)
> +{
> +	struct task_struct *target_task;
> +	int ret;
> +
> +	if (status != 0 || target_pid == 0)
> +		return 0;

[Severity: Medium]
Could this condition cause the kworker program to silently run during other
remote tests and invalidate their test coverage?

During existing remote tests, both the pid and target_pid global variables
are set to non-zero values. Because this program is auto-attached by
test_send_signal_kern__attach() and only checks target_pid == 0, it appears
it will run and send the signal when background workqueue events occur
during those other tests.

If this program delivers the signal and sets status = 1, it could preempt
the intended test programs (like send_signal_tp or send_signal_perf),
causing the remote tests to spuriously pass without actually exercising
their specific BPF programs.

Would it make sense to also verify pid == 0 here to guarantee this program
remains inert when the other tests are running?

> +
> +	target_task = bpf_task_from_pid(target_pid);
> +	if (!target_task)
> +		return 0;
> +
> +	ret = bpf_send_signal_task(target_task, sig, PIDTYPE_TGID, 8);
> +	bpf_task_release(target_task);
> +
> +	if (ret == 0)
> +		status = 1;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902150439.692725-1-adi.sharma@zohomail.in?part=2