| Message ID | 20241007103426.128923-2-puranjay@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Implement mechanism to signal other threads | expand |
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
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 >
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
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
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
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();
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(-)