On Thu, Sep 5, 2024 at 1:56 AM Puranjay Mohan <puranjay@xxxxxxxxxx> wrote:
>
> 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?
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
