On Wed, Sep 4, 2024 at 6:23 AM Puranjay Mohan <puranjay@xxxxxxxxxx> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes:
>
> Hi,
> Sorry for the delay on this.
>
> > On Wed, Jul 24, 2024 at 4:40 AM Puranjay Mohan <puranjay@xxxxxxxxxx> wrote:
> >>
> >> Implement bpf_send_signal_pid and bpf_send_signal_tgid helpers which are
> >> similar to bpf_send_signal_thread and bpf_send_signal helpers
> >> respectively but can be used to send signals to other threads and
> >> processes.
> >
> > Thanks for working on this!
> > But it needs more homework.
> >
> >> #define ___BPF_FUNC_MAPPER(FN, ctx...) \
> >> FN(unspec, 0, ##ctx) \
> >> @@ -6006,6 +6041,8 @@ union bpf_attr {
> >> FN(user_ringbuf_drain, 209, ##ctx) \
> >> FN(cgrp_storage_get, 210, ##ctx) \
> >> FN(cgrp_storage_delete, 211, ##ctx) \
> >> + FN(send_signal_pid, 212, ##ctx) \
> >> + FN(send_signal_tgid, 213, ##ctx) \
> >
> > We stopped adding helpers long ago.
> > They need to be kfuncs.
> >
> >> /* */
> >>
> >> /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >> index cd098846e251..f1e58122600d 100644
> >> --- a/kernel/trace/bpf_trace.c
> >> +++ b/kernel/trace/bpf_trace.c
> >> @@ -839,21 +839,30 @@ static void do_bpf_send_signal(struct irq_work *entry)
> >> 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, u32 pid)
> >> {
> >> struct send_signal_irq_work *work = NULL;
> >> + struct task_struct *tsk;
> >> +
> >> + if (pid) {
> >> + tsk = find_task_by_vpid(pid);
> >
> > by vpid ?
> >
> > tracing bpf prog will have "random" current and "random" pidns.
> >
> > Should it be find_get_task vs find_task too ?
> >
> > Should kfunc take 'task' parameter instead
> > received from bpf_task_from_pid() ?
> >
> > two kfuncs for pid/tgid is overkill. Combine into one?
>
> So, I will add a single kfunc that can do both pid and tgid and it will
> take the 'task' parameter received from the call to bpf_task_from_pid()
> and a 'bool' to select pid/tgid.
Can you please also investigate passing an extra u64 of "context" to
the signal handler? It's been requested before, and at least for some
signals the kernel seems to support this functionality. Would be best
to avoid proliferation of kfuncs, if we can handle all this in one.
>
> >
> >> + if (!tsk)
> >> + return -ESRCH;
> >> + } else {
> >> + tsk = current;
> >> + }
>
> Thanks,
> Puranjay
