diff mbox series

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

Message ID 20240724113944.75977-1-puranjay@kernel.org (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,1/2] bpf: implement bpf_send_signal_pid/tgid() helpers | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 898 this patch: 898
netdev/build_tools success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers success CCed 18 of 18 maintainers
netdev/build_clang success Errors and warnings before: 971 this patch: 971
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7630 this patch: 7630
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 203 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat

Commit Message

Puranjay Mohan July 24, 2024, 11:39 a.m. UTC 
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.

Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 include/uapi/linux/bpf.h       | 37 ++++++++++++++++++++++++
 kernel/trace/bpf_trace.c       | 53 +++++++++++++++++++++++++++++-----
 tools/include/uapi/linux/bpf.h | 37 ++++++++++++++++++++++++
 3 files changed, 120 insertions(+), 7 deletions(-)

Comments

Alexei Starovoitov July 24, 2024, 11:23 p.m. UTC | #1 
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?

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

pw-bot: cr
Puranjay Mohan Sept. 4, 2024, 1:23 p.m. UTC | #2 
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
Alexei Starovoitov Sept. 4, 2024, 3:36 p.m. UTC | #3 
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.

why bool ?
enum pid_type is kernel enum that is available to bpf progs
via vmlinux.h. The prog can just pass that directly.
Do you see any safety issue that it will pass PIDTYPE_MAX ?
If so that can be an additional check inside kfunc.
Andrii Nakryiko Sept. 4, 2024, 5:55 p.m. UTC | #4 
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.

>
> >
> >> +               if (!tsk)
> >> +                       return -ESRCH;
> >> +       } else {
> >> +               tsk = current;
> >> +       }
>
> Thanks,
> Puranjay
Puranjay Mohan Sept. 5, 2024, 8:56 a.m. UTC | #5 
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
Andrii Nakryiko Sept. 5, 2024, 8:41 p.m. UTC | #6 
On Thu, Sep 5, 2024 at 1:56 AM Puranjay Mohan <puranjay@kernel.org> wrote:
>
> 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?

There must be. I think last time I looked at this, I found this (from [0]):

       •  If the signal is sent using sigqueue(3), an accompanying value
          (either an integer or a pointer) can be sent with the signal.
          If the receiving process establishes a handler for this signal
          using the SA_SIGINFO flag to sigaction(2), then it can obtain
          this data via the si_value field of the siginfo_t structure
          passed as the second argument to the handler.  Furthermore,
          the si_pid and si_uid fields of this structure can be used to
          obtain the PID and real user ID of the process sending the
          signal.

And there were some kernel-side parts related to that, yes. But please
take a look yourself and check if it's all sane.

  [0] https://man7.org/linux/man-pages/man7/signal.7.html


>
> Thanks,
> Puranjay
diff mbox series

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 35bcf52dbc65..7b29003c079c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5792,6 +5792,41 @@  union bpf_attr {
  *		0 on success.
  *
  *		**-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * long bpf_send_signal_pid(u32 sig, u32 pid)
+ *	Description
+ *		Send signal *sig* to the thread corresponding to the
+ *		process id *pid*.
+ *	Return
+ *		0 on success or successfully queued.
+ *
+ *		**-EBUSY** if work queue under nmi is full.
+ *
+ *		**-EINVAL** if *sig* is invalid.
+ *
+ *		**-EPERM** if no permission to send the *sig*.
+ *
+ *		**-EAGAIN** if bpf program can try again.
+ *
+ *		**-ESRCH** if *pid* is invalid.
+ *
+ * long bpf_send_signal_tgid(u32 sig, u32 tgid)
+ *	Description
+ *		Send signal *sig* to the process corresponding to the
+ *		thread group id *tgid*.
+ *		The signal may be delivered to any of this process's threads.
+ *	Return
+ *		0 on success or successfully queued.
+ *
+ *		**-EBUSY** if work queue under nmi is full.
+ *
+ *		**-EINVAL** if *sig* is invalid.
+ *
+ *		**-EPERM** if no permission to send the *sig*.
+ *
+ *		**-EAGAIN** if bpf program can try again.
+ *
+ *		**-ESRCH** if *tgid* is invalid.
  */
 #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)		\
 	/* */
 
 /* 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);
+		if (!tsk)
+			return -ESRCH;
+	} else {
+		tsk = current;
+	}
 
 	/* Similar to bpf_probe_write_user, task needs to be
 	 * in a sound condition and kernel memory access be
 	 * permitted in order to send signal to the current
 	 * task.
 	 */
-	if (unlikely(current->flags & (PF_KTHREAD | PF_EXITING)))
+	if (unlikely(tsk->flags & (PF_KTHREAD | PF_EXITING)))
 		return -EPERM;
 	if (unlikely(!nmi_uaccess_okay()))
 		return -EPERM;
 	/* Task should not be pid=1 to avoid kernel panic. */
-	if (unlikely(is_global_init(current)))
+	if (unlikely(is_global_init(tsk)))
 		return -EPERM;
 
 	if (irqs_disabled()) {
@@ -871,19 +880,19 @@  static int bpf_send_signal_common(u32 sig, enum pid_type type)
 		 * to the irq_work. The current task may change when queued
 		 * irq works get executed.
 		 */
-		work->task = get_task_struct(current);
+		work->task = get_task_struct(tsk);
 		work->sig = sig;
 		work->type = type;
 		irq_work_queue(&work->irq_work);
 		return 0;
 	}
 
-	return group_send_sig_info(sig, SEND_SIG_PRIV, current, type);
+	return group_send_sig_info(sig, SEND_SIG_PRIV, tsk, type);
 }
 
 BPF_CALL_1(bpf_send_signal, u32, sig)
 {
-	return bpf_send_signal_common(sig, PIDTYPE_TGID);
+	return bpf_send_signal_common(sig, PIDTYPE_TGID, 0);
 }
 
 static const struct bpf_func_proto bpf_send_signal_proto = {
@@ -895,7 +904,7 @@  static const struct bpf_func_proto bpf_send_signal_proto = {
 
 BPF_CALL_1(bpf_send_signal_thread, u32, sig)
 {
-	return bpf_send_signal_common(sig, PIDTYPE_PID);
+	return bpf_send_signal_common(sig, PIDTYPE_PID, 0);
 }
 
 static const struct bpf_func_proto bpf_send_signal_thread_proto = {
@@ -905,6 +914,32 @@  static const struct bpf_func_proto bpf_send_signal_thread_proto = {
 	.arg1_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_2(bpf_send_signal_pid, u32, sig, u32, pid)
+{
+	return bpf_send_signal_common(sig, PIDTYPE_PID, pid);
+}
+
+static const struct bpf_func_proto bpf_send_signal_pid_proto = {
+	.func		= bpf_send_signal_pid,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+	.arg2_type	= ARG_ANYTHING,
+};
+
+BPF_CALL_2(bpf_send_signal_tgid, u32, sig, u32, tgid)
+{
+	return bpf_send_signal_common(sig, PIDTYPE_TGID, tgid);
+}
+
+static const struct bpf_func_proto bpf_send_signal_tgid_proto = {
+	.func		= bpf_send_signal_tgid,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+	.arg2_type	= ARG_ANYTHING,
+};
+
 BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
 {
 	struct path copy;
@@ -1583,6 +1618,10 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_send_signal_proto;
 	case BPF_FUNC_send_signal_thread:
 		return &bpf_send_signal_thread_proto;
+	case BPF_FUNC_send_signal_pid:
+		return &bpf_send_signal_pid_proto;
+	case BPF_FUNC_send_signal_tgid:
+		return &bpf_send_signal_tgid_proto;
 	case BPF_FUNC_perf_event_read_value:
 		return &bpf_perf_event_read_value_proto;
 	case BPF_FUNC_ringbuf_output:
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 35bcf52dbc65..7b29003c079c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5792,6 +5792,41 @@  union bpf_attr {
  *		0 on success.
  *
  *		**-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * long bpf_send_signal_pid(u32 sig, u32 pid)
+ *	Description
+ *		Send signal *sig* to the thread corresponding to the
+ *		process id *pid*.
+ *	Return
+ *		0 on success or successfully queued.
+ *
+ *		**-EBUSY** if work queue under nmi is full.
+ *
+ *		**-EINVAL** if *sig* is invalid.
+ *
+ *		**-EPERM** if no permission to send the *sig*.
+ *
+ *		**-EAGAIN** if bpf program can try again.
+ *
+ *		**-ESRCH** if *pid* is invalid.
+ *
+ * long bpf_send_signal_tgid(u32 sig, u32 tgid)
+ *	Description
+ *		Send signal *sig* to the process corresponding to the
+ *		thread group id *tgid*.
+ *		The signal may be delivered to any of this process's threads.
+ *	Return
+ *		0 on success or successfully queued.
+ *
+ *		**-EBUSY** if work queue under nmi is full.
+ *
+ *		**-EINVAL** if *sig* is invalid.
+ *
+ *		**-EPERM** if no permission to send the *sig*.
+ *
+ *		**-EAGAIN** if bpf program can try again.
+ *
+ *		**-ESRCH** if *tgid* is invalid.
  */
 #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)		\
 	/* */
 
 /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't