Re: [PATCH bpf-next 1/2] bpf: implement bpf_send_signal_pid/tgid() helpers

[Puranjay Mohan <puranjay@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3570 bytes --]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Wed, Sep 4, 2024 at 6:23 AM Puranjay Mohan <puranjay@kernel.org> wrote:
>>
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>
>> Hi,
>> Sorry for the delay on this.
>>
>> > On Wed, Jul 24, 2024 at 4:40 AM Puranjay Mohan <puranjay@kernel.org> 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.
>

Yes, I will look into that. Are you referring to the 'void *context'
that is passed to the handlers registered with sigaction()? like:

--- 8< ---

void  handle_prof_signal(int signal, siginfo_t * info, void * context)
{
}

struct sigaction sig_action;
struct sigaction old_action;

memset(&sig_action, 0, sizeof(sig_action));

sig_action.sa_sigaction = handle_prof_signal;
sig_action.sa_flags = SA_RESTART | SA_SIGINFO;
sigemptyset(&sig_action.sa_mask);

sigaction(SIGPROF, &sig_action, &old_action);

--- >8 ---

And we want to the BPF program to also be able to pass a custom context
to the signal handler like above? is there an existing mechanism to do
that in the kernel?

Thanks,
Puranjay

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 255 bytes --]