[PATCH bpf-next v3 0/2] Implement mechanism to signal other threads

[PATCH bpf-next v3 0/2] Implement mechanism to signal other threads

[Puranjay Mohan]

This set implements a kfunc called bpf_send_signal_task() that is similar
to sigqueue() as it can send a signal along with a cookie to a thread or
thread group.

The send_signal selftest has been updated to also test this new kfunc under
all contexts.

Changes in v3:
v2: https://lore.kernel.org/all/20240926115328.105634-1-puranjay@kernel.org/
- make the cookie u64 instead of int.
- re use code from bpf_send_signal_common

Changes in v2:
v1: https://lore.kernel.org/bpf/20240724113944.75977-1-puranjay@kernel.org/
- Convert to a kfunc
- Add mechanism to send a cookie with the signal.

Puranjay Mohan (2):
  bpf: implement bpf_send_signal_task() kfunc
  selftests/bpf: Augment send_signal test with remote signaling

 kernel/bpf/helpers.c                          |   1 +
 kernel/trace/bpf_trace.c                      |  54 +++++--
 .../selftests/bpf/prog_tests/send_signal.c    | 133 +++++++++++++-----
 .../bpf/progs/test_send_signal_kern.c         |  35 ++++-
 4 files changed, 177 insertions(+), 46 deletions(-)

-- 
2.40.1

[PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc

[Puranjay Mohan]

Implement bpf_send_signal_task kfunc that is similar to
bpf_send_signal_thread and bpf_send_signal helpers  but can be used to
send signals to other threads and processes. It also supports sending a
cookie with the signal similar to sigqueue().

If the receiving process establishes a handler for the signal using the
SA_SIGINFO flag to sigaction(), then it can obtain this cookie via the
si_value field of the siginfo_t structure passed as the second argument
to the handler.

Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 kernel/bpf/helpers.c     |  1 +
 kernel/trace/bpf_trace.c | 54 ++++++++++++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 4053f279ed4cc..2fd3feefb9d94 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3035,6 +3035,7 @@ BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
 #endif
 BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_throw)
+BTF_ID_FLAGS(func, bpf_send_signal_task, KF_TRUSTED_ARGS)
 BTF_KFUNCS_END(generic_btf_ids)
 
 static const struct btf_kfunc_id_set generic_kfunc_set = {
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a582cd25ca876..ae8c9fa8b04d1 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -802,6 +802,8 @@ struct send_signal_irq_work {
 	struct task_struct *task;
 	u32 sig;
 	enum pid_type type;
+	bool has_siginfo;
+	kernel_siginfo_t info;
 };
 
 static DEFINE_PER_CPU(struct send_signal_irq_work, send_signal_work);
@@ -811,25 +813,43 @@ static void do_bpf_send_signal(struct irq_work *entry)
 	struct send_signal_irq_work *work;
 
 	work = container_of(entry, struct send_signal_irq_work, irq_work);
-	group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
+	if (work->has_siginfo)
+		group_send_sig_info(work->sig, &work->info, work->task, work->type);
+	else
+		group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
 	put_task_struct(work->task);
 }
 
-static int bpf_send_signal_common(u32 sig, enum pid_type type)
+static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struct *tsk, u64 value)
 {
 	struct send_signal_irq_work *work = NULL;
+	kernel_siginfo_t info;
+	bool has_siginfo = false;
+
+	if (!tsk) {
+		tsk = current;
+	} else {
+		has_siginfo = true;
+		clear_siginfo(&info);
+		info.si_signo = sig;
+		info.si_errno = 0;
+		info.si_code = SI_KERNEL;
+		info.si_pid = 0;
+		info.si_uid = 0;
+		info.si_value.sival_ptr = (void *)value;
+	}
 
 	/* Similar to bpf_probe_write_user, task needs to be
 	 * in a sound condition and kernel memory access be
 	 * permitted in order to send signal to the current
 	 * task.
 	 */
-	if (unlikely(current->flags & (PF_KTHREAD | PF_EXITING)))
+	if (unlikely(tsk->flags & (PF_KTHREAD | PF_EXITING)))
 		return -EPERM;
 	if (unlikely(!nmi_uaccess_okay()))
 		return -EPERM;
 	/* Task should not be pid=1 to avoid kernel panic. */
-	if (unlikely(is_global_init(current)))
+	if (unlikely(is_global_init(tsk)))
 		return -EPERM;
 
 	if (irqs_disabled()) {
@@ -847,19 +867,24 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type)
 		 * to the irq_work. The current task may change when queued
 		 * irq works get executed.
 		 */
-		work->task = get_task_struct(current);
+		work->task = get_task_struct(tsk);
+		work->has_siginfo = has_siginfo;
+		work->info = info;
 		work->sig = sig;
 		work->type = type;
 		irq_work_queue(&work->irq_work);
 		return 0;
 	}
 
-	return group_send_sig_info(sig, SEND_SIG_PRIV, current, type);
+	if (has_siginfo)
+		return group_send_sig_info(sig, &info, tsk, type);
+
+	return group_send_sig_info(sig, SEND_SIG_PRIV, tsk, type);
 }
 
 BPF_CALL_1(bpf_send_signal, u32, sig)
 {
-	return bpf_send_signal_common(sig, PIDTYPE_TGID);
+	return bpf_send_signal_common(sig, PIDTYPE_TGID, NULL, 0);
 }
 
 static const struct bpf_func_proto bpf_send_signal_proto = {
@@ -871,7 +896,7 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
 
 BPF_CALL_1(bpf_send_signal_thread, u32, sig)
 {
-	return bpf_send_signal_common(sig, PIDTYPE_PID);
+	return bpf_send_signal_common(sig, PIDTYPE_PID, NULL, 0);
 }
 
 static const struct bpf_func_proto bpf_send_signal_thread_proto = {
@@ -3484,3 +3509,16 @@ static int __init bpf_kprobe_multi_kfuncs_init(void)
 }
 
 late_initcall(bpf_kprobe_multi_kfuncs_init);
+
+__bpf_kfunc_start_defs();
+
+__bpf_kfunc int bpf_send_signal_task(struct task_struct *task, int sig, enum pid_type type,
+				     u64 value)
+{
+	if (type != PIDTYPE_PID && type != PIDTYPE_TGID)
+		return -EINVAL;
+
+	return bpf_send_signal_common(sig, type, task, value);
+}
+
+__bpf_kfunc_end_defs();
-- 
2.40.1

[PATCH bpf-next v3 2/2] selftests/bpf: Augment send_signal test with remote signaling

[Puranjay Mohan]

Add testcases to test bpf_send_signal_task(). In these new test cases,
the main process triggers the BPF program and the forked process
receives the signals. The target process's signal handler receives a
cookie from the bpf program.

Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 .../selftests/bpf/prog_tests/send_signal.c    | 133 +++++++++++++-----
 .../bpf/progs/test_send_signal_kern.c         |  35 ++++-
 2 files changed, 130 insertions(+), 38 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 6cc69900b3106..beb771347a503 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -8,17 +8,25 @@ static int sigusr1_received;
 
 static void sigusr1_handler(int signum)
 {
-	sigusr1_received = 1;
+	sigusr1_received = 8;
+}
+
+static void sigusr1_siginfo_handler(int s, siginfo_t *i, void *v)
+{
+	sigusr1_received = i->si_value.sival_int;
 }
 
 static void test_send_signal_common(struct perf_event_attr *attr,
-				    bool signal_thread)
+				    bool signal_thread, bool remote)
 {
 	struct test_send_signal_kern *skel;
+	struct sigaction sa;
 	int pipe_c2p[2], pipe_p2c[2];
 	int err = -1, pmu_fd = -1;
+	volatile int j = 0;
 	char buf[256];
 	pid_t pid;
+	int old_prio;
 
 	if (!ASSERT_OK(pipe(pipe_c2p), "pipe_c2p"))
 		return;
@@ -39,11 +47,14 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	}
 
 	if (pid == 0) {
-		int old_prio;
-		volatile int j = 0;
-
 		/* install signal handler and notify parent */
-		ASSERT_NEQ(signal(SIGUSR1, sigusr1_handler), SIG_ERR, "signal");
+		if (remote) {
+			sa.sa_sigaction = sigusr1_siginfo_handler;
+			sa.sa_flags = SA_RESTART | SA_SIGINFO;
+			ASSERT_NEQ(sigaction(SIGUSR1, &sa, NULL), -1, "sigaction");
+		} else {
+			ASSERT_NEQ(signal(SIGUSR1, sigusr1_handler), SIG_ERR, "signal");
+		}
 
 		close(pipe_c2p[0]); /* close read */
 		close(pipe_p2c[1]); /* close write */
@@ -52,10 +63,12 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 		 * that if an interrupt happens, the underlying task
 		 * is this process.
 		 */
-		errno = 0;
-		old_prio = getpriority(PRIO_PROCESS, 0);
-		ASSERT_OK(errno, "getpriority");
-		ASSERT_OK(setpriority(PRIO_PROCESS, 0, -20), "setpriority");
+		if (!remote) {
+			errno = 0;
+			old_prio = getpriority(PRIO_PROCESS, 0);
+			ASSERT_OK(errno, "getpriority");
+			ASSERT_OK(setpriority(PRIO_PROCESS, 0, -20), "setpriority");
+		}
 
 		/* notify parent signal handler is installed */
 		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
@@ -66,20 +79,25 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 		/* wait a little for signal handler */
 		for (int i = 0; i < 1000000000 && !sigusr1_received; i++) {
 			j /= i + j + 1;
-			if (!attr)
-				/* trigger the nanosleep tracepoint program. */
-				usleep(1);
+			if (remote)
+				sleep(1);
+			else
+				if (!attr)
+					/* trigger the nanosleep tracepoint program. */
+					usleep(1);
 		}
 
-		buf[0] = sigusr1_received ? '2' : '0';
-		ASSERT_EQ(sigusr1_received, 1, "sigusr1_received");
+		buf[0] = sigusr1_received;
+
+		ASSERT_EQ(sigusr1_received, 8, "sigusr1_received");
 		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
 
 		/* wait for parent notification and exit */
 		ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
 
 		/* restore the old priority */
-		ASSERT_OK(setpriority(PRIO_PROCESS, 0, old_prio), "setpriority");
+		if (!remote)
+			ASSERT_OK(setpriority(PRIO_PROCESS, 0, old_prio), "setpriority");
 
 		close(pipe_c2p[1]);
 		close(pipe_p2c[0]);
@@ -93,6 +111,17 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
 		goto skel_open_load_failure;
 
+	/* boost with a high priority so we got a higher chance
+	 * that if an interrupt happens, the underlying task
+	 * is this process.
+	 */
+	if (remote) {
+		errno = 0;
+		old_prio = getpriority(PRIO_PROCESS, 0);
+		ASSERT_OK(errno, "getpriority");
+		ASSERT_OK(setpriority(PRIO_PROCESS, 0, -20), "setpriority");
+	}
+
 	if (!attr) {
 		err = test_send_signal_kern__attach(skel);
 		if (!ASSERT_OK(err, "skel_attach")) {
@@ -100,8 +129,12 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 			goto destroy_skel;
 		}
 	} else {
-		pmu_fd = syscall(__NR_perf_event_open, attr, pid, -1 /* cpu */,
-				 -1 /* group id */, 0 /* flags */);
+		if (!remote)
+			pmu_fd = syscall(__NR_perf_event_open, attr, pid, -1 /* cpu */,
+					 -1 /* group id */, 0 /* flags */);
+		else
+			pmu_fd = syscall(__NR_perf_event_open, attr, getpid(), -1 /* cpu */,
+					 -1 /* group id */, 0 /* flags */);
 		if (!ASSERT_GE(pmu_fd, 0, "perf_event_open")) {
 			err = -1;
 			goto destroy_skel;
@@ -119,11 +152,30 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	/* trigger the bpf send_signal */
 	skel->bss->signal_thread = signal_thread;
 	skel->bss->sig = SIGUSR1;
-	skel->bss->pid = pid;
+	if (!remote) {
+		skel->bss->target_pid = 0;
+		skel->bss->pid = pid;
+	} else {
+		skel->bss->target_pid = pid;
+		skel->bss->pid = getpid();
+	}
 
 	/* notify child that bpf program can send_signal now */
 	ASSERT_EQ(write(pipe_p2c[1], buf, 1), 1, "pipe_write");
 
+	/* For the remote test, the BPF program is triggered from this
+	 * process but the other process/thread is signaled.
+	 */
+	if (remote) {
+		if (!attr) {
+			for (int i = 0; i < 10; i++)
+				usleep(1);
+		} else {
+			for (int i = 0; i < 100000000; i++)
+				j /= i + 1;
+		}
+	}
+
 	/* wait for result */
 	err = read(pipe_c2p[0], buf, 1);
 	if (!ASSERT_GE(err, 0, "reading pipe"))
@@ -133,7 +185,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 		goto disable_pmu;
 	}
 
-	ASSERT_EQ(buf[0], '2', "incorrect result");
+	ASSERT_EQ(buf[0], 8, "incorrect result");
 
 	/* notify child safe to exit */
 	ASSERT_EQ(write(pipe_p2c[1], buf, 1), 1, "pipe_write");
@@ -142,18 +194,21 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	close(pmu_fd);
 destroy_skel:
 	test_send_signal_kern__destroy(skel);
+	/* restore the old priority */
+	if (remote)
+		ASSERT_OK(setpriority(PRIO_PROCESS, 0, old_prio), "setpriority");
 skel_open_load_failure:
 	close(pipe_c2p[0]);
 	close(pipe_p2c[1]);
 	wait(NULL);
 }
 
-static void test_send_signal_tracepoint(bool signal_thread)
+static void test_send_signal_tracepoint(bool signal_thread, bool remote)
 {
-	test_send_signal_common(NULL, signal_thread);
+	test_send_signal_common(NULL, signal_thread, remote);
 }
 
-static void test_send_signal_perf(bool signal_thread)
+static void test_send_signal_perf(bool signal_thread, bool remote)
 {
 	struct perf_event_attr attr = {
 		.freq = 1,
@@ -162,10 +217,10 @@ static void test_send_signal_perf(bool signal_thread)
 		.config = PERF_COUNT_SW_CPU_CLOCK,
 	};
 
-	test_send_signal_common(&attr, signal_thread);
+	test_send_signal_common(&attr, signal_thread, remote);
 }
 
-static void test_send_signal_nmi(bool signal_thread)
+static void test_send_signal_nmi(bool signal_thread, bool remote)
 {
 	struct perf_event_attr attr = {
 		.sample_period = 1,
@@ -191,21 +246,35 @@ static void test_send_signal_nmi(bool signal_thread)
 		close(pmu_fd);
 	}
 
-	test_send_signal_common(&attr, signal_thread);
+	test_send_signal_common(&attr, signal_thread, remote);
 }
 
 void test_send_signal(void)
 {
 	if (test__start_subtest("send_signal_tracepoint"))
-		test_send_signal_tracepoint(false);
+		test_send_signal_tracepoint(false, false);
 	if (test__start_subtest("send_signal_perf"))
-		test_send_signal_perf(false);
+		test_send_signal_perf(false, false);
 	if (test__start_subtest("send_signal_nmi"))
-		test_send_signal_nmi(false);
+		test_send_signal_nmi(false, false);
 	if (test__start_subtest("send_signal_tracepoint_thread"))
-		test_send_signal_tracepoint(true);
+		test_send_signal_tracepoint(true, false);
 	if (test__start_subtest("send_signal_perf_thread"))
-		test_send_signal_perf(true);
+		test_send_signal_perf(true, false);
 	if (test__start_subtest("send_signal_nmi_thread"))
-		test_send_signal_nmi(true);
+		test_send_signal_nmi(true, false);
+
+	/* Signal remote thread and thread group */
+	if (test__start_subtest("send_signal_tracepoint_remote"))
+		test_send_signal_tracepoint(false, true);
+	if (test__start_subtest("send_signal_perf_remote"))
+		test_send_signal_perf(false, true);
+	if (test__start_subtest("send_signal_nmi_remote"))
+		test_send_signal_nmi(false, true);
+	if (test__start_subtest("send_signal_tracepoint_thread_remote"))
+		test_send_signal_tracepoint(true, true);
+	if (test__start_subtest("send_signal_perf_thread_remote"))
+		test_send_signal_perf(true, true);
+	if (test__start_subtest("send_signal_nmi_thread_remote"))
+		test_send_signal_nmi(true, true);
 }
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 92354cd720440..176a355e30624 100644
--- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
@@ -1,27 +1,50 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2019 Facebook
-#include <linux/bpf.h>
+#include <vmlinux.h>
 #include <linux/version.h>
 #include <bpf/bpf_helpers.h>
 
-__u32 sig = 0, pid = 0, status = 0, signal_thread = 0;
+struct task_struct *bpf_task_from_pid(int pid) __ksym;
+void bpf_task_release(struct task_struct *p) __ksym;
+int bpf_send_signal_task(struct task_struct *task, int sig, enum pid_type type, u64 value) __ksym;
+
+__u32 sig = 0, pid = 0, status = 0, signal_thread = 0, target_pid = 0;
 
 static __always_inline int bpf_send_signal_test(void *ctx)
 {
+	struct task_struct *target_task = NULL;
 	int ret;
+	u64 value;
 
 	if (status != 0 || pid == 0)
 		return 0;
 
 	if ((bpf_get_current_pid_tgid() >> 32) == pid) {
-		if (signal_thread)
-			ret = bpf_send_signal_thread(sig);
-		else
-			ret = bpf_send_signal(sig);
+		if (target_pid) {
+			target_task = bpf_task_from_pid(target_pid);
+			if (!target_task)
+				return 0;
+			value = 8;
+		}
+
+		if (signal_thread) {
+			if (target_pid)
+				ret = bpf_send_signal_task(target_task, sig, PIDTYPE_PID, value);
+			else
+				ret = bpf_send_signal_thread(sig);
+		} else {
+			if (target_pid)
+				ret = bpf_send_signal_task(target_task, sig, PIDTYPE_TGID, value);
+			else
+				ret = bpf_send_signal(sig);
+		}
 		if (ret == 0)
 			status = 1;
 	}
 
+	if (target_task)
+		bpf_task_release(target_task);
+
 	return 0;
 }
 
-- 
2.40.1

Re: [PATCH bpf-next v3 2/2] selftests/bpf: Augment send_signal test with remote signaling

[Puranjay Mohan]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

Puranjay Mohan <puranjay@kernel.org> writes:

> Add testcases to test bpf_send_signal_task(). In these new test cases,
> the main process triggers the BPF program and the forked process
> receives the signals. The target process's signal handler receives a
> cookie from the bpf program.
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/send_signal.c    | 133 +++++++++++++-----
>  .../bpf/progs/test_send_signal_kern.c         |  35 ++++-
>  2 files changed, 130 insertions(+), 38 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 6cc69900b3106..beb771347a503 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -8,17 +8,25 @@ static int sigusr1_received;
>  
>  static void sigusr1_handler(int signum)
>  {
> -	sigusr1_received = 1;
> +	sigusr1_received = 8;
> +}
> +
> +static void sigusr1_siginfo_handler(int s, siginfo_t *i, void *v)
> +{
> +	sigusr1_received = i->si_value.sival_int;

This is incorrect for big-endian archs and I will change this to:

     sigusr1_received = (int)(long long)i->si_value.sival_ptr;

This should work on all archs.

>  }
>  
>  static void test_send_signal_common(struct perf_event_attr *attr,
> -				    bool signal_thread)
> +				    bool signal_thread, bool remote)

[...]

Thanks,
Puranjay

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 255 bytes --]

Re: [PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc

[kernel test robot]

Hi Puranjay,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Puranjay-Mohan/bpf-implement-bpf_send_signal_task-kfunc/20241007-183648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20241007103426.128923-2-puranjay%40kernel.org
patch subject: [PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc
config: i386-randconfig-141-20241008 (https://download.01.org/0day-ci/archive/20241008/202410080907.DFxFxfor-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241008/202410080907.DFxFxfor-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410080907.DFxFxfor-lkp@intel.com/

All warnings (new ones prefixed by >>):

   kernel/trace/bpf_trace.c: In function 'bpf_send_signal_common':
>> kernel/trace/bpf_trace.c:839:43: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     839 |                 info.si_value.sival_ptr = (void *)value;
         |                                           ^


vim +839 kernel/trace/bpf_trace.c

   822	
   823	static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struct *tsk, u64 value)
   824	{
   825		struct send_signal_irq_work *work = NULL;
   826		kernel_siginfo_t info;
   827		bool has_siginfo = false;
   828	
   829		if (!tsk) {
   830			tsk = current;
   831		} else {
   832			has_siginfo = true;
   833			clear_siginfo(&info);
   834			info.si_signo = sig;
   835			info.si_errno = 0;
   836			info.si_code = SI_KERNEL;
   837			info.si_pid = 0;
   838			info.si_uid = 0;
 > 839			info.si_value.sival_ptr = (void *)value;
   840		}
   841	
   842		/* Similar to bpf_probe_write_user, task needs to be
   843		 * in a sound condition and kernel memory access be
   844		 * permitted in order to send signal to the current
   845		 * task.
   846		 */
   847		if (unlikely(tsk->flags & (PF_KTHREAD | PF_EXITING)))
   848			return -EPERM;
   849		if (unlikely(!nmi_uaccess_okay()))
   850			return -EPERM;
   851		/* Task should not be pid=1 to avoid kernel panic. */
   852		if (unlikely(is_global_init(tsk)))
   853			return -EPERM;
   854	
   855		if (irqs_disabled()) {
   856			/* Do an early check on signal validity. Otherwise,
   857			 * the error is lost in deferred irq_work.
   858			 */
   859			if (unlikely(!valid_signal(sig)))
   860				return -EINVAL;
   861	
   862			work = this_cpu_ptr(&send_signal_work);
   863			if (irq_work_is_busy(&work->irq_work))
   864				return -EBUSY;
   865	
   866			/* Add the current task, which is the target of sending signal,
   867			 * to the irq_work. The current task may change when queued
   868			 * irq works get executed.
   869			 */
   870			work->task = get_task_struct(tsk);
   871			work->has_siginfo = has_siginfo;
   872			work->info = info;
   873			work->sig = sig;
   874			work->type = type;
   875			irq_work_queue(&work->irq_work);
   876			return 0;
   877		}
   878	
   879		if (has_siginfo)
   880			return group_send_sig_info(sig, &info, tsk, type);
   881	
   882		return group_send_sig_info(sig, SEND_SIG_PRIV, tsk, type);
   883	}
   884	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc

[Andrii Nakryiko]

On Mon, Oct 7, 2024 at 3:34 AM Puranjay Mohan <puranjay@kernel.org> wrote:
>
> Implement bpf_send_signal_task kfunc that is similar to
> bpf_send_signal_thread and bpf_send_signal helpers  but can be used to
> send signals to other threads and processes. It also supports sending a
> cookie with the signal similar to sigqueue().
>
> If the receiving process establishes a handler for the signal using the
> SA_SIGINFO flag to sigaction(), then it can obtain this cookie via the
> si_value field of the siginfo_t structure passed as the second argument
> to the handler.
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  kernel/bpf/helpers.c     |  1 +
>  kernel/trace/bpf_trace.c | 54 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 4053f279ed4cc..2fd3feefb9d94 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -3035,6 +3035,7 @@ BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
>  #endif
>  BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_throw)
> +BTF_ID_FLAGS(func, bpf_send_signal_task, KF_TRUSTED_ARGS)
>  BTF_KFUNCS_END(generic_btf_ids)
>
>  static const struct btf_kfunc_id_set generic_kfunc_set = {
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a582cd25ca876..ae8c9fa8b04d1 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -802,6 +802,8 @@ struct send_signal_irq_work {
>         struct task_struct *task;
>         u32 sig;
>         enum pid_type type;
> +       bool has_siginfo;
> +       kernel_siginfo_t info;

group_send_sig_info() refers to this as `struct kernel_siginfo`, let's
use that and avoid unnecessary typedefs

>  };
>
>  static DEFINE_PER_CPU(struct send_signal_irq_work, send_signal_work);
> @@ -811,25 +813,43 @@ static void do_bpf_send_signal(struct irq_work *entry)
>         struct send_signal_irq_work *work;
>
>         work = container_of(entry, struct send_signal_irq_work, irq_work);
> -       group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
> +       if (work->has_siginfo)
> +               group_send_sig_info(work->sig, &work->info, work->task, work->type);
> +       else
> +               group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);

There is lots of duplication while the only difference is between
providing SEND_SIG_PRIV and our own &work->info. So maybe let's just
have something like

struct kernel_siginfo *siginfo;

siginfo = work->has_siginfo ? &work->info : SEND_SIG_PRIV;
group_send_sig_info(work->sig, siginfo, work->task, work->type);

?

>         put_task_struct(work->task);
>  }
>
> -static int bpf_send_signal_common(u32 sig, enum pid_type type)
> +static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struct *tsk, u64 value)

task? why tsk?

>  {
>         struct send_signal_irq_work *work = NULL;
> +       kernel_siginfo_t info;
> +       bool has_siginfo = false;
> +
> +       if (!tsk) {
> +               tsk = current;
> +       } else {
> +               has_siginfo = true;

nit: I find it less confusing for cases like with has_siginfo here,
for the variable to be explicitly assigned in both branches, instead
of defaulting to false and then reassigned in one of the branches

> +               clear_siginfo(&info);
> +               info.si_signo = sig;
> +               info.si_errno = 0;
> +               info.si_code = SI_KERNEL;
> +               info.si_pid = 0;
> +               info.si_uid = 0;
> +               info.si_value.sival_ptr = (void *)value;
> +       }

kernel test bot complains that this should probably be (void
*)(unsigned long)value (which will truncate on 32-bit archtes, but oh
well)

but can you please double check that it's ok to set
info.si_value.sival_ptr for any signal? Because si_value.sival_ptr is
actually defined inside __sifields._rt._sigval, which clearly would
conflict with _kill, _timer, _sigchld and other groups of signals.

so I suspect we'd need to have a list of signals that are OK accepting
this extra u64 value, and reject it otherwise (instead of silently
corrupting data inside __sifields

pw-bot: cr

>
>         /* Similar to bpf_probe_write_user, task needs to be
>          * in a sound condition and kernel memory access be
>          * permitted in order to send signal to the current
>          * task.
>          */
> -       if (unlikely(current->flags & (PF_KTHREAD | PF_EXITING)))
> +       if (unlikely(tsk->flags & (PF_KTHREAD | PF_EXITING)))
>                 return -EPERM;
>         if (unlikely(!nmi_uaccess_okay()))
>                 return -EPERM;
>         /* Task should not be pid=1 to avoid kernel panic. */
> -       if (unlikely(is_global_init(current)))
> +       if (unlikely(is_global_init(tsk)))
>                 return -EPERM;
>
>         if (irqs_disabled()) {
> @@ -847,19 +867,24 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type)
>                  * to the irq_work. The current task may change when queued
>                  * irq works get executed.
>                  */
> -               work->task = get_task_struct(current);
> +               work->task = get_task_struct(tsk);
> +               work->has_siginfo = has_siginfo;
> +               work->info = info;

if you are using clear_siginfo(), you probably should use copy_siginfo() here?

>                 work->sig = sig;
>                 work->type = type;
>                 irq_work_queue(&work->irq_work);
>                 return 0;
>         }
>
> -       return group_send_sig_info(sig, SEND_SIG_PRIV, current, type);
> +       if (has_siginfo)
> +               return group_send_sig_info(sig, &info, tsk, type);
> +
> +       return group_send_sig_info(sig, SEND_SIG_PRIV, tsk, type);

Similarly to what I mentioned at the very top, the only difference is
a pointer to struct kernel_siginfo, so make it explicit?

struct kernel_siginfo *siginfo;

siginfo = task == current ? SEND_SIG_PRIV : &info;

?

>  }
>
>  BPF_CALL_1(bpf_send_signal, u32, sig)
>  {
> -       return bpf_send_signal_common(sig, PIDTYPE_TGID);
> +       return bpf_send_signal_common(sig, PIDTYPE_TGID, NULL, 0);
>  }
>
>  static const struct bpf_func_proto bpf_send_signal_proto = {
> @@ -871,7 +896,7 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
>
>  BPF_CALL_1(bpf_send_signal_thread, u32, sig)
>  {
> -       return bpf_send_signal_common(sig, PIDTYPE_PID);
> +       return bpf_send_signal_common(sig, PIDTYPE_PID, NULL, 0);
>  }
>
>  static const struct bpf_func_proto bpf_send_signal_thread_proto = {
> @@ -3484,3 +3509,16 @@ static int __init bpf_kprobe_multi_kfuncs_init(void)
>  }
>
>  late_initcall(bpf_kprobe_multi_kfuncs_init);
> +
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc int bpf_send_signal_task(struct task_struct *task, int sig, enum pid_type type,
> +                                    u64 value)
> +{
> +       if (type != PIDTYPE_PID && type != PIDTYPE_TGID)
> +               return -EINVAL;
> +
> +       return bpf_send_signal_common(sig, type, task, value);
> +}
> +
> +__bpf_kfunc_end_defs();
> --
> 2.40.1
>

Re: [PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc

[kernel test robot]

Hi Puranjay,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Puranjay-Mohan/bpf-implement-bpf_send_signal_task-kfunc/20241007-183648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20241007103426.128923-2-puranjay%40kernel.org
patch subject: [PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc
config: x86_64-randconfig-121-20241008 (https://download.01.org/0day-ci/archive/20241008/202410081401.fYRMhL8t-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241008/202410081401.fYRMhL8t-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410081401.fYRMhL8t-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> kernel/trace/bpf_trace.c:839:41: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void [noderef] __user *[addressable] [assigned] [usertype] sival_ptr @@     got void * @@
   kernel/trace/bpf_trace.c:839:41: sparse:     expected void [noderef] __user *[addressable] [assigned] [usertype] sival_ptr
   kernel/trace/bpf_trace.c:839:41: sparse:     got void *
   kernel/trace/bpf_trace.c: note: in included file (through include/linux/smp.h, include/linux/lockdep.h, include/linux/spinlock.h, ...):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   kernel/trace/bpf_trace.c: note: in included file (through include/linux/rbtree.h, include/linux/mm_types.h, include/linux/mmzone.h, ...):
   include/linux/rcupdate.h:880:25: sparse: sparse: context imbalance in 'uprobe_prog_run' - unexpected unlock

vim +839 kernel/trace/bpf_trace.c

   822	
   823	static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struct *tsk, u64 value)
   824	{
   825		struct send_signal_irq_work *work = NULL;
   826		kernel_siginfo_t info;
   827		bool has_siginfo = false;
   828	
   829		if (!tsk) {
   830			tsk = current;
   831		} else {
   832			has_siginfo = true;
   833			clear_siginfo(&info);
   834			info.si_signo = sig;
   835			info.si_errno = 0;
   836			info.si_code = SI_KERNEL;
   837			info.si_pid = 0;
   838			info.si_uid = 0;
 > 839			info.si_value.sival_ptr = (void *)value;
   840		}
   841	
   842		/* Similar to bpf_probe_write_user, task needs to be
   843		 * in a sound condition and kernel memory access be
   844		 * permitted in order to send signal to the current
   845		 * task.
   846		 */
   847		if (unlikely(tsk->flags & (PF_KTHREAD | PF_EXITING)))
   848			return -EPERM;
   849		if (unlikely(!nmi_uaccess_okay()))
   850			return -EPERM;
   851		/* Task should not be pid=1 to avoid kernel panic. */
   852		if (unlikely(is_global_init(tsk)))
   853			return -EPERM;
   854	
   855		if (irqs_disabled()) {
   856			/* Do an early check on signal validity. Otherwise,
   857			 * the error is lost in deferred irq_work.
   858			 */
   859			if (unlikely(!valid_signal(sig)))
   860				return -EINVAL;
   861	
   862			work = this_cpu_ptr(&send_signal_work);
   863			if (irq_work_is_busy(&work->irq_work))
   864				return -EBUSY;
   865	
   866			/* Add the current task, which is the target of sending signal,
   867			 * to the irq_work. The current task may change when queued
   868			 * irq works get executed.
   869			 */
   870			work->task = get_task_struct(tsk);
   871			work->has_siginfo = has_siginfo;
   872			work->info = info;
   873			work->sig = sig;
   874			work->type = type;
   875			irq_work_queue(&work->irq_work);
   876			return 0;
   877		}
   878	
   879		if (has_siginfo)
   880			return group_send_sig_info(sig, &info, tsk, type);
   881	
   882		return group_send_sig_info(sig, SEND_SIG_PRIV, tsk, type);
   883	}
   884	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc

[Puranjay Mohan]

On Tue, Oct 8, 2024 at 6:24 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Oct 7, 2024 at 3:34 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> >
> > Implement bpf_send_signal_task kfunc that is similar to
> > bpf_send_signal_thread and bpf_send_signal helpers  but can be used to
> > send signals to other threads and processes. It also supports sending a
> > cookie with the signal similar to sigqueue().
> >
> > If the receiving process establishes a handler for the signal using the
> > SA_SIGINFO flag to sigaction(), then it can obtain this cookie via the
> > si_value field of the siginfo_t structure passed as the second argument
> > to the handler.
> >
> > Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> > ---
> >  kernel/bpf/helpers.c     |  1 +
> >  kernel/trace/bpf_trace.c | 54 ++++++++++++++++++++++++++++++++++------
> >  2 files changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 4053f279ed4cc..2fd3feefb9d94 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -3035,6 +3035,7 @@ BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
> >  #endif
> >  BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
> >  BTF_ID_FLAGS(func, bpf_throw)
> > +BTF_ID_FLAGS(func, bpf_send_signal_task, KF_TRUSTED_ARGS)
> >  BTF_KFUNCS_END(generic_btf_ids)
> >
> >  static const struct btf_kfunc_id_set generic_kfunc_set = {
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index a582cd25ca876..ae8c9fa8b04d1 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -802,6 +802,8 @@ struct send_signal_irq_work {
> >         struct task_struct *task;
> >         u32 sig;
> >         enum pid_type type;
> > +       bool has_siginfo;
> > +       kernel_siginfo_t info;
>
> group_send_sig_info() refers to this as `struct kernel_siginfo`, let's
> use that and avoid unnecessary typedefs
>
> >  };
> >
> >  static DEFINE_PER_CPU(struct send_signal_irq_work, send_signal_work);
> > @@ -811,25 +813,43 @@ static void do_bpf_send_signal(struct irq_work *entry)
> >         struct send_signal_irq_work *work;
> >
> >         work = container_of(entry, struct send_signal_irq_work, irq_work);
> > -       group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
> > +       if (work->has_siginfo)
> > +               group_send_sig_info(work->sig, &work->info, work->task, work->type);
> > +       else
> > +               group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
>
> There is lots of duplication while the only difference is between
> providing SEND_SIG_PRIV and our own &work->info. So maybe let's just
> have something like
>
> struct kernel_siginfo *siginfo;
>
> siginfo = work->has_siginfo ? &work->info : SEND_SIG_PRIV;
> group_send_sig_info(work->sig, siginfo, work->task, work->type);
>
> ?
>
> >         put_task_struct(work->task);
> >  }
> >
> > -static int bpf_send_signal_common(u32 sig, enum pid_type type)
> > +static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struct *tsk, u64 value)
>
> task? why tsk?
>
> >  {
> >         struct send_signal_irq_work *work = NULL;
> > +       kernel_siginfo_t info;
> > +       bool has_siginfo = false;
> > +
> > +       if (!tsk) {
> > +               tsk = current;
> > +       } else {
> > +               has_siginfo = true;
>
> nit: I find it less confusing for cases like with has_siginfo here,
> for the variable to be explicitly assigned in both branches, instead
> of defaulting to false and then reassigned in one of the branches
>
> > +               clear_siginfo(&info);
> > +               info.si_signo = sig;
> > +               info.si_errno = 0;
> > +               info.si_code = SI_KERNEL;
> > +               info.si_pid = 0;
> > +               info.si_uid = 0;
> > +               info.si_value.sival_ptr = (void *)value;
> > +       }
>
> kernel test bot complains that this should probably be (void
> *)(unsigned long)value (which will truncate on 32-bit archtes, but oh
> well)
>
> but can you please double check that it's ok to set
> info.si_value.sival_ptr for any signal? Because si_value.sival_ptr is
> actually defined inside __sifields._rt._sigval, which clearly would
> conflict with _kill, _timer, _sigchld and other groups of signals.
>
> so I suspect we'd need to have a list of signals that are OK accepting
> this extra u64 value, and reject it otherwise (instead of silently
> corrupting data inside __sifields

I tried reading the man pages of sigqueue and it allows using all signals.

To test it, I sent SIGCHLD to a process with si_value.sival_ptr using
sigqueue() and it worked as expected.

It shouldn't affect us as we are not populating all fields of
__sifields anyway. For example if you send SIGCHLD using
this new kfunc, there is no way to set _utime and _stime or even _pid
and _uid, here only the signal number
and this u64 value is relevant.

I will make all the other suggested changes in the next version.


Thanks,
Puranjay

Re: [PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc

[Andrii Nakryiko]

On Tue, Oct 8, 2024 at 3:17 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> On Tue, Oct 8, 2024 at 6:24 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Oct 7, 2024 at 3:34 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> > >
> > > Implement bpf_send_signal_task kfunc that is similar to
> > > bpf_send_signal_thread and bpf_send_signal helpers  but can be used to
> > > send signals to other threads and processes. It also supports sending a
> > > cookie with the signal similar to sigqueue().
> > >
> > > If the receiving process establishes a handler for the signal using the
> > > SA_SIGINFO flag to sigaction(), then it can obtain this cookie via the
> > > si_value field of the siginfo_t structure passed as the second argument
> > > to the handler.
> > >
> > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> > > ---
> > >  kernel/bpf/helpers.c     |  1 +
> > >  kernel/trace/bpf_trace.c | 54 ++++++++++++++++++++++++++++++++++------
> > >  2 files changed, 47 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index 4053f279ed4cc..2fd3feefb9d94 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -3035,6 +3035,7 @@ BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
> > >  #endif
> > >  BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
> > >  BTF_ID_FLAGS(func, bpf_throw)
> > > +BTF_ID_FLAGS(func, bpf_send_signal_task, KF_TRUSTED_ARGS)
> > >  BTF_KFUNCS_END(generic_btf_ids)
> > >
> > >  static const struct btf_kfunc_id_set generic_kfunc_set = {
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index a582cd25ca876..ae8c9fa8b04d1 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -802,6 +802,8 @@ struct send_signal_irq_work {
> > >         struct task_struct *task;
> > >         u32 sig;
> > >         enum pid_type type;
> > > +       bool has_siginfo;
> > > +       kernel_siginfo_t info;
> >
> > group_send_sig_info() refers to this as `struct kernel_siginfo`, let's
> > use that and avoid unnecessary typedefs
> >
> > >  };
> > >
> > >  static DEFINE_PER_CPU(struct send_signal_irq_work, send_signal_work);
> > > @@ -811,25 +813,43 @@ static void do_bpf_send_signal(struct irq_work *entry)
> > >         struct send_signal_irq_work *work;
> > >
> > >         work = container_of(entry, struct send_signal_irq_work, irq_work);
> > > -       group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
> > > +       if (work->has_siginfo)
> > > +               group_send_sig_info(work->sig, &work->info, work->task, work->type);
> > > +       else
> > > +               group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
> >
> > There is lots of duplication while the only difference is between
> > providing SEND_SIG_PRIV and our own &work->info. So maybe let's just
> > have something like
> >
> > struct kernel_siginfo *siginfo;
> >
> > siginfo = work->has_siginfo ? &work->info : SEND_SIG_PRIV;
> > group_send_sig_info(work->sig, siginfo, work->task, work->type);
> >
> > ?
> >
> > >         put_task_struct(work->task);
> > >  }
> > >
> > > -static int bpf_send_signal_common(u32 sig, enum pid_type type)
> > > +static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struct *tsk, u64 value)
> >
> > task? why tsk?
> >
> > >  {
> > >         struct send_signal_irq_work *work = NULL;
> > > +       kernel_siginfo_t info;
> > > +       bool has_siginfo = false;
> > > +
> > > +       if (!tsk) {
> > > +               tsk = current;
> > > +       } else {
> > > +               has_siginfo = true;
> >
> > nit: I find it less confusing for cases like with has_siginfo here,
> > for the variable to be explicitly assigned in both branches, instead
> > of defaulting to false and then reassigned in one of the branches
> >
> > > +               clear_siginfo(&info);
> > > +               info.si_signo = sig;
> > > +               info.si_errno = 0;
> > > +               info.si_code = SI_KERNEL;
> > > +               info.si_pid = 0;
> > > +               info.si_uid = 0;
> > > +               info.si_value.sival_ptr = (void *)value;
> > > +       }
> >
> > kernel test bot complains that this should probably be (void
> > *)(unsigned long)value (which will truncate on 32-bit archtes, but oh
> > well)
> >
> > but can you please double check that it's ok to set
> > info.si_value.sival_ptr for any signal? Because si_value.sival_ptr is
> > actually defined inside __sifields._rt._sigval, which clearly would
> > conflict with _kill, _timer, _sigchld and other groups of signals.
> >
> > so I suspect we'd need to have a list of signals that are OK accepting
> > this extra u64 value, and reject it otherwise (instead of silently
> > corrupting data inside __sifields
>
> I tried reading the man pages of sigqueue and it allows using all signals.
>
> To test it, I sent SIGCHLD to a process with si_value.sival_ptr using
> sigqueue() and it worked as expected.
>
> It shouldn't affect us as we are not populating all fields of
> __sifields anyway. For example if you send SIGCHLD using

But __sifields is *a union*, where there is a separate struct for
kill, separate for timer signals, separate for POSIX.1b signals, and
yet another struct (inside the union, so they are all mutually
exclusive) for SIGCHLD. Then another group for SIGILL, SIGFPE,
SIGSEGV, SIGBUS, SIGTRAP, SIGEMT. SIGPOLL is separate, and SIGSYS is
separate still.

So I'm confused. Sure, C will allow you to set _rt._sigval fields, but
the question is which part of that union is the kernel using for
different signals? Or are you saying that whatever is sent with
group_send_sig_info() will use __sifields._rt part of the union,
regardless of the actual signal?

> this new kfunc, there is no way to set _utime and _stime or even _pid
> and _uid, here only the signal number
> and this u64 value is relevant.
>
> I will make all the other suggested changes in the next version.
>
>
> Thanks,
> Puranjay