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@xxxxxxxxx> wrote:
>
> On Mon, Oct 7, 2024 at 3:34 AM Puranjay Mohan <puranjay@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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