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.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 996C4C43381 for ; Wed, 27 Mar 2019 20:45:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 592242075E for ; Wed, 27 Mar 2019 20:45:28 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=netflix.com header.i=@netflix.com header.b="LVTHdv9T" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729404AbfC0Up1 (ORCPT ); Wed, 27 Mar 2019 16:45:27 -0400 Received: from mail-wr1-f68.google.com ([209.85.221.68]:37015 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726344AbfC0Up1 (ORCPT ); Wed, 27 Mar 2019 16:45:27 -0400 Received: by mail-wr1-f68.google.com with SMTP id w10so20206199wrm.4 for ; Wed, 27 Mar 2019 13:45:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netflix.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=bkWRdNkz9t1HjRPdj0yZTKQhebCASVaEQXU0vlH1gD0=; b=LVTHdv9Tl5RBmFbTYjV8A4B1k255pyK6kRTm9IrOXwVDsjMdXfUn7yQS77tbt47JSf 41s6/be7lcR2zopCWF3DeEsSsdOKIj74qHzAWV0fLkLBXIuwzRlGCktBD4Z1S29XkN3n JFKFNm7q+BOOK7ZlDTPG8/GzJ9/e3F7zAT1RU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bkWRdNkz9t1HjRPdj0yZTKQhebCASVaEQXU0vlH1gD0=; b=C8KJzrlnEbkNMewfNvhC04dhIJ63UmoG7dXDwKsJP8jtrjGwb0PjqTJO9pUd/Cbq8l 0PVXzoOoHaTMvMWPxIVMh8oT2Xesy3MEDPCCa08SKq4jS6Ok1GTPrB0bonU4mrirEG8g OM2soMEzbmOj5g4tSBFQxtMG6OfJhgaz5sAWPICBQTxz8gxzgYtlY/wqn0drqwXbqQW+ q26jlH702bNcFDhBFmoZKESMGJFvrhthbK18LiudXxC15hYMMSfAQXGBJvJvDHPn5oki A0qi5AFn2d1piKJkYcQFFdB6vAEQoJClqKsp03i7IWJ9ZYE9BicQEUONM274t4uGFx76 jnkw== X-Gm-Message-State: APjAAAWulZinWF/6PRC7Y4xpgLybhOXaYaZZUy6sisxW7FYmoqyorXpQ +fUKTX1EHbmm8ZUT7CsJCVWdAxkoVpj9pDVTtDyiOA== X-Google-Smtp-Source: APXvYqwafbvltkbui2Hxyxym/JXj23hlGZZWQvhVDfxO41f9jXS2dv0UBUuh9U94siWtYGCtQvww3LaEU4ltq7RHnwE= X-Received: by 2002:a5d:5190:: with SMTP id k16mr7100060wrv.10.1553719524568; Wed, 27 Mar 2019 13:45:24 -0700 (PDT) MIME-Version: 1.0 References: <20190305224759.2555660-1-javierhonduco@fb.com> <20190322223848.3338614-1-javierhonduco@fb.com> <20190322223848.3338614-2-javierhonduco@fb.com> <123407cd-f439-7158-1c06-7cd6f10ba8e9@iogearbox.net> <20190327155446.GA63993@fb.com> In-Reply-To: <20190327155446.GA63993@fb.com> From: Brendan Gregg Date: Wed, 27 Mar 2019 13:44:58 -0700 Message-ID: Subject: Re: [PATCH v4 bpf-next 1/3] bpf: add bpf_progenyof helper To: Javier Honduvilla Coto Cc: Daniel Borkmann , "netdev@vger.kernel.org" , Yonghong Song , Kernel Team Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Mar 27, 2019 at 8:59 AM Javier Honduvilla Coto wrote: > > On Mon, Mar 25, 2019 at 03:17:03PM +0100, Daniel Borkmann wrote: > > On 03/22/2019 11:38 PM, Javier Honduvilla Coto wrote: > > > This patch adds the bpf_progenyof helper which receives a PID and returns > > > 1 if the process currently being executed is in the process hierarchy > > > including itself or 0 if not. > > > > > > This is very useful in tracing programs when we want to filter by a > > > given PID and all the children it might spawn. The current workarounds > > > most people implement for this purpose have issues: > > > > > > - Attaching to process spawning syscalls and dynamically add those PIDs > > > to some bpf map that would be used to filter is cumbersome and > > > potentially racy. > > > - Unrolling some loop to perform what this helper is doing consumes lots > > > of instructions. That and the impossibility to jump backwards makes it > > > really hard to be correct in really large process chains. > > > > > > Signed-off-by: Javier Honduvilla Coto > > > --- > > > include/linux/bpf.h | 1 + > > > include/uapi/linux/bpf.h | 10 +++++++++- > > > kernel/bpf/core.c | 1 + > > > kernel/bpf/helpers.c | 32 ++++++++++++++++++++++++++++++++ > > > kernel/trace/bpf_trace.c | 2 ++ > > > 5 files changed, 45 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > index f62897198844..bd0d2b38e7d5 100644 > > > --- a/include/linux/bpf.h > > > +++ b/include/linux/bpf.h > > > @@ -930,6 +930,7 @@ extern const struct bpf_func_proto bpf_sk_redirect_map_proto; > > > extern const struct bpf_func_proto bpf_spin_lock_proto; > > > extern const struct bpf_func_proto bpf_spin_unlock_proto; > > > extern const struct bpf_func_proto bpf_get_local_storage_proto; > > > +extern const struct bpf_func_proto bpf_progenyof_proto; > > > > > > /* Shared helpers among cBPF and eBPF. */ > > > void bpf_user_rnd_init_once(void); > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > index 3c04410137d9..cf54cc739bf4 100644 > > > --- a/include/uapi/linux/bpf.h > > > +++ b/include/uapi/linux/bpf.h > > > @@ -2463,6 +2463,13 @@ union bpf_attr { > > > * Return > > > * 0 if iph and th are a valid SYN cookie ACK, or a negative error > > > * otherwise. > > > + * int bpf_progenyof(int pid) > > > + * Description > > > + * This helper is useful in programs that want to filter events > > > + * happening to a pid of any of its descendants. > > > + * Return > > > + * 1 if the currently executing process' pid is in the process > > > + * hierarchy of the passed pid. 0 Otherwise. > > > > What about the -EINVAL? > > I think we can remove this, as Alexei told me this was not needed > anymore (copied it from other helpers that have it). > > Should we remove that check in other patch for the other helpers > that have it? > > > > > > */ > > > #define __BPF_FUNC_MAPPER(FN) \ > > > FN(unspec), \ > > > @@ -2565,7 +2572,8 @@ union bpf_attr { > > > FN(skb_ecn_set_ce), \ > > > FN(get_listener_sock), \ > > > FN(skc_lookup_tcp), \ > > > - FN(tcp_check_syncookie), > > > + FN(tcp_check_syncookie), \ > > > + FN(progenyof), > > > > > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > > > * function eBPF program intends to call > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > > > index ff09d32a8a1b..437986497468 100644 > > > --- a/kernel/bpf/core.c > > > +++ b/kernel/bpf/core.c > > > @@ -2044,6 +2044,7 @@ const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak; > > > const struct bpf_func_proto bpf_get_current_comm_proto __weak; > > > const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak; > > > const struct bpf_func_proto bpf_get_local_storage_proto __weak; > > > +const struct bpf_func_proto bpf_progenyof_proto __weak; > > > > > > const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void) > > > { > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > > index a411fc17d265..f093b35d1ba8 100644 > > > --- a/kernel/bpf/helpers.c > > > +++ b/kernel/bpf/helpers.c > > > @@ -18,6 +18,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > /* If kernel subsystem is allowing eBPF programs to call this function, > > > * inside its own verifier_ops->get_func_proto() callback it should return > > > @@ -364,3 +365,34 @@ const struct bpf_func_proto bpf_get_local_storage_proto = { > > > }; > > > #endif > > > #endif > > > + > > > +BPF_CALL_1(bpf_progenyof, int, pid) > > > > Nit: could we add a more descriptive helper name? What's progenyof? Also s/int/pid_t/? > > > > It's true that "progeny" is not a very commonly used word :D. A coworker > suggested "descendantof", what do you think? It's a bit difficult to > convey "is true on the passed pid + on all the process under the > hierarchy chain of that pid", so I will try rewording the docs so the > semantics are very clear! What about childof? I see the word child used more than anything: $ man ps pgrep pidstat pstree top | grep child | wc -l 29 $ man ps pgrep pidstat pstree top | grep progeny | wc -l 0 $ man ps pgrep pidstat pstree top | grep ancestor | wc -l 2 Brendan -- Brendan Gregg, Senior Performance Architect, Netflix