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@xxxxxxxxx> wrote:
>
> 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

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