Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
> 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.
>
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:
signature.asc
Description: PGP signature
