[v3] Implement mechanism to signal other threads

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

Posted by Puranjay Mohan 1 month, 3 weeks ago

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

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

Posted by kernel test robot 1 month, 2 weeks ago

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

Posted by Andrii Nakryiko 1 month, 2 weeks ago

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

Posted by Puranjay Mohan 1 month, 2 weeks ago

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

Posted by Andrii Nakryiko 1 month, 2 weeks ago

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

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

Posted by kernel test robot 1 month, 2 weeks ago

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

[PATCH bpf-next v3 1/2] bpf: implement bpf_send_signal_task() kfunc
[PATCH bpf-next v3 2/2] selftests/bpf: Augment send_signal test with remote signaling