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: 2399 bytes --]

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.

>
>> +               if (!tsk)
>> +                       return -ESRCH;
>> +       } else {
>> +               tsk = current;
>> +       }

Thanks,
Puranjay

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