From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 06779C282E5 for ; Fri, 24 May 2019 21:32:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D1B4D20879 for ; Fri, 24 May 2019 21:32:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404237AbfEXVcI (ORCPT ); Fri, 24 May 2019 17:32:08 -0400 Received: from www62.your-server.de ([213.133.104.62]:51302 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2391752AbfEXVcI (ORCPT ); Fri, 24 May 2019 17:32:08 -0400 Received: from [78.46.172.3] (helo=sslproxy06.your-server.de) by www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1) (envelope-from ) id 1hUHnJ-0000fj-7u; Fri, 24 May 2019 23:32:05 +0200 Received: from [178.197.249.12] (helo=linux.home) by sslproxy06.your-server.de with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from ) id 1hUHnJ-000LuF-01; Fri, 24 May 2019 23:32:05 +0200 Subject: Re: [PATCH bpf-next v2 1/3] bpf: implement bpf_send_signal() helper To: Yonghong Song , "bpf@vger.kernel.org" , "netdev@vger.kernel.org" Cc: Alexei Starovoitov , Kernel Team , Peter Zijlstra References: <20190522053900.1663459-1-yhs@fb.com> <20190522053900.1663537-1-yhs@fb.com> <2c07890b-9da5-b4e8-dc94-35def14470ad@iogearbox.net> <6041511a-1628-868f-b4b1-e567c234a4a5@fb.com> <86aacfb6-614b-55cb-7fe8-9f2c5c63c126@fb.com> <5384a86c-94a4-f60f-2414-b8c68d152f57@iogearbox.net> <99fb7b8f-e2b0-4601-bc3c-3fa8e250c15d@fb.com> From: Daniel Borkmann Message-ID: Date: Fri, 24 May 2019 23:32:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <99fb7b8f-e2b0-4601-bc3c-3fa8e250c15d@fb.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.100.3/25459/Fri May 24 09:59:21 2019) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 05/24/2019 01:54 AM, Yonghong Song wrote: > > > On 5/23/19 4:08 PM, Daniel Borkmann wrote: >> On 05/23/2019 11:30 PM, Yonghong Song wrote: >>> On 5/23/19 2:07 PM, Yonghong Song wrote: >>>> On 5/23/19 9:28 AM, Daniel Borkmann wrote: >>>>> On 05/23/2019 05:58 PM, Yonghong Song wrote: >>>>>> On 5/23/19 8:41 AM, Daniel Borkmann wrote: >>>>>>> On 05/22/2019 07:39 AM, Yonghong Song wrote: >>>>>>>> This patch tries to solve the following specific use case. >>>>>>>> >>>>>>>> Currently, bpf program can already collect stack traces >>>>>>>> through kernel function get_perf_callchain() >>>>>>>> when certain events happens (e.g., cache miss counter or >>>>>>>> cpu clock counter overflows). But such stack traces are >>>>>>>> not enough for jitted programs, e.g., hhvm (jited php). >>>>>>>> To get real stack trace, jit engine internal data structures >>>>>>>> need to be traversed in order to get the real user functions. >>>>>>>> >>>>>>>> bpf program itself may not be the best place to traverse >>>>>>>> the jit engine as the traversing logic could be complex and >>>>>>>> it is not a stable interface either. >>>>>>>> >>>>>>>> Instead, hhvm implements a signal handler, >>>>>>>> e.g. for SIGALARM, and a set of program locations which >>>>>>>> it can dump stack traces. When it receives a signal, it will >>>>>>>> dump the stack in next such program location. >>>>>>>> >>>>>>>> Such a mechanism can be implemented in the following way: >>>>>>>> . a perf ring buffer is created between bpf program >>>>>>>> and tracing app. >>>>>>>> . once a particular event happens, bpf program writes >>>>>>>> to the ring buffer and the tracing app gets notified. >>>>>>>> . the tracing app sends a signal SIGALARM to the hhvm. >>>>>>>> >>>>>>>> But this method could have large delays and causing profiling >>>>>>>> results skewed. >>>>>>>> >>>>>>>> This patch implements bpf_send_signal() helper to send >>>>>>>> a signal to hhvm in real time, resulting in intended stack traces. >>>>>>>> >>>>>>>> Signed-off-by: Yonghong Song >>>>>>>> --- >>>>>>>> include/uapi/linux/bpf.h | 17 +++++++++- >>>>>>>> kernel/trace/bpf_trace.c | 67 ++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 2 files changed, 83 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >>>>>>>> index 63e0cf66f01a..68d4470523a0 100644 >>>>>>>> --- a/include/uapi/linux/bpf.h >>>>>>>> +++ b/include/uapi/linux/bpf.h >>>>>>>> @@ -2672,6 +2672,20 @@ union bpf_attr { >>>>>>>> * 0 on success. >>>>>>>> * >>>>>>>> * **-ENOENT** if the bpf-local-storage cannot be found. >>>>>>>> + * >>>>>>>> + * int bpf_send_signal(u32 sig) >>>>>>>> + * Description >>>>>>>> + * Send signal *sig* to the current task. >>>>>>>> + * 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. >>>>>>>> */ >>>>>>>> #define __BPF_FUNC_MAPPER(FN) \ >>>>>>>> FN(unspec), \ >>>>>>>> @@ -2782,7 +2796,8 @@ union bpf_attr { >>>>>>>> FN(strtol), \ >>>>>>>> FN(strtoul), \ >>>>>>>> FN(sk_storage_get), \ >>>>>>>> - FN(sk_storage_delete), >>>>>>>> + FN(sk_storage_delete), \ >>>>>>>> + FN(send_signal), >>>>>>>> >>>>>>>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper >>>>>>>> * function eBPF program intends to call >>>>>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>>>>>>> index f92d6ad5e080..f8cd0db7289f 100644 >>>>>>>> --- a/kernel/trace/bpf_trace.c >>>>>>>> +++ b/kernel/trace/bpf_trace.c >>>>>>>> @@ -567,6 +567,58 @@ static const struct bpf_func_proto bpf_probe_read_str_proto = { >>>>>>>> .arg3_type = ARG_ANYTHING, >>>>>>>> }; >>>>>>>> >>>>>>>> +struct send_signal_irq_work { >>>>>>>> + struct irq_work irq_work; >>>>>>>> + u32 sig; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static DEFINE_PER_CPU(struct send_signal_irq_work, send_signal_work); >>>>>>>> + >>>>>>>> +static void do_bpf_send_signal(struct irq_work *entry) >>>>>>>> +{ >>>>>>>> + struct send_signal_irq_work *work; >>>>>>>> + >>>>>>>> + work = container_of(entry, struct send_signal_irq_work, irq_work); >>>>>>>> + group_send_sig_info(work->sig, SEND_SIG_PRIV, current, PIDTYPE_TGID); >>>>>>>> +} >>>>>>>> + >>>>>>>> +BPF_CALL_1(bpf_send_signal, u32, sig) >>>>>>>> +{ >>>>>>>> + struct send_signal_irq_work *work = NULL; >>>>>>>> + >>>>>>>> + /* 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))) >>>>>>>> + return -EPERM; >>>>>>>> + if (unlikely(uaccess_kernel())) >>>>>>>> + return -EPERM; >>>>>>>> + if (unlikely(!nmi_uaccess_okay())) >>>>>>>> + return -EPERM; >>>>>>>> + >>>>>>>> + if (in_nmi()) { >>>>>>> >>>>>>> Hm, bit confused, can't this only be done out of process context in >>>>>>> general since only there current points to e.g. hhvm? I'm probably >>>>>>> missing something. Could you elaborate? >>>>>> >>>>>> That is true. If in nmi, it is out of process context and in nmi >>>>>> context, we use an irq_work here since group_send_sig_info() has >>>>>> spinlock inside. The bpf program (e.g., a perf_event program) needs to >>>>>> check it is with right current (e.g., by pid) before calling >>>>>> this helper. >>>>>> >>>>>> Does this address your question? >>>> >>>> Thanks, Daniel. The below are really good questions which I did not >>>> really think through with irq_work. >>>> >>>>> Hm, but how is it guaranteed that 'current' inside the callback is still >>>>> the very same you intend to send the signal to? >>>> >>>> I went through irq_work infrastructure. It looks that "current" may >>>> change. irq_work is registered as an interrupt on x86. >>>> After nmi, some lower priority >>>> interrupts get chances to execute including irq_work. But there are some >>>> other interrupts like timer_interrupt and reschedule_interrupt may >>>> change "current". But since we are still in interrupt context, task >>>> should not be destroyed, so the task structure pointer is still valid. >>>> >>>> I will pass "current" task struct pointer to irq_work as well. This >>>> is similar to what we did in stackmap.c: >>>> work->sem = ¤t->mm->mmap_sem; >>>> irq_work_queue(&work->irq_work); >>>> At irq_work_run() time, the previous "current" in nmi should still be >>>> valid. >>>> >>>>> What happens if you're in softirq and send SIGKILL to yourself? Is this >>>>> ignored/handled gracefully in such case? >>>> >>>> It is not ignored. It handled gracefully in this case. I tried my >>>> example to send SIGKILL. The call stack looks below. >>>> >>>> [ 24.211943] bpf_send_signal+0x9/0xb0 >>>> [ 24.212427] bpf_prog_fec6e7cc664d5b91_bpf_send_signal_test+0x228/0x1000 >>>> [ 24.213249] ? bpf_overflow_handler+0xb7/0x180 >>>> [ 24.213853] ? __perf_event_overflow+0x51/0xe0 >>>> [ 24.214385] ? perf_swevent_hrtimer+0x10a/0x160 >>>> [ 24.214965] ? __update_load_avg_cfs_rq+0x1a9/0x1c0 >>>> [ 24.215609] ? task_tick_fair+0x50/0x690 >>>> [ 24.216104] ? run_timer_softirq+0x208/0x490 >>>> [ 24.216637] ? timerqueue_del+0x1e/0x40 >>>> [ 24.217111] ? task_clock_event_del+0x10/0x10 >>>> [ 24.217658] ? __hrtimer_run_queues+0x10d/0x2c0 >>>> [ 24.218217] ? hrtimer_interrupt+0x122/0x270 >>>> [ 24.218765] ? rcu_irq_enter+0x31/0x110 >>>> [ 24.219223] ? smp_apic_timer_interrupt+0x67/0x160 >>>> [ 24.219842] ? apic_timer_interrupt+0xf/0x20 >>>> [ 24.220383] >>>> [ 24.220655] ? event_sched_out.isra.108+0x150/0x150 >>>> [ 24.221271] ? smp_call_function_single+0xdc/0x100 >>>> [ 24.221898] ? perf_event_sysfs_show+0x20/0x20 >>>> [ 24.222469] ? cpu_function_call+0x42/0x60 >>>> [ 24.222982] ? cpu_clock_event_read+0x10/0x10 >>>> [ 24.223525] ? event_function_call+0xe6/0xf0 >>>> [ 24.224053] ? event_sched_out.isra.108+0x150/0x150 >>>> [ 24.224657] ? perf_remove_from_context+0x20/0x70 >>>> [ 24.225262] ? perf_event_release_kernel+0x106/0x2e0 >>>> [ 24.225896] ? perf_release+0xc/0x10 >>>> [ 24.226347] ? __fput+0xc9/0x230 >>>> [ 24.226767] ? task_work_run+0x83/0xb0 >>>> [ 24.227243] ? do_exit+0x300/0xc50 >>>> [ 24.227674] ? syscall_trace_enter+0x1c9/0x2d0 >>>> [ 24.228223] ? do_group_exit+0x39/0xb0 >>>> [ 24.228695] ? __x64_sys_exit_group+0x14/0x20 >>>> [ 24.229270] ? do_syscall_64+0x49/0x130 >>>> [ 24.229762] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>> >>>> I see the task is killed and other process is not impacted and >>>> no kernel crash/warning. >> >> Hm, but I rather meant when you have the case that we're in_serving_softirq() >> e.g. when processing packets on rx and you send a signal to yourself. Shouldn't >> we bail out from the helper in such situation as well? > > Just want to clarify. Are you concerned with safety or correctness? > > For safety, if we do send signal here, we may wreck the system? > > For correctness, you mean the information we got to send a signal > to process is not quite right if in_serving_softirq()? F.e, > the performance counter overflow may be caused by softirq rather > the process itself? So in this case, we should only send signal > to process when in process context, and in nmi (not serving softirq)? > > If for correctness, do you think we should add a "flags" parameter > to the bpf_send_signal() helper such that: > . default not checking is_serving_softirq() > . bit0: if set, bail out if is_serving_softirq() > . other bits: reserved Scratch my thought, we do bail out in case of PF_KTHREAD, so should be okay. Was thinking in terms of both, not wrecking the system / messing with kthreads and with regards to correctness.