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=-6.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,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 DB6A0C433DF for ; Thu, 18 Jun 2020 17:53:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B1D1320734 for ; Thu, 18 Jun 2020 17:53:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="iLqGHLCu" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729112AbgFRRxa (ORCPT ); Thu, 18 Jun 2020 13:53:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58530 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726976AbgFRRx1 (ORCPT ); Thu, 18 Jun 2020 13:53:27 -0400 Received: from mail-qk1-x742.google.com (mail-qk1-x742.google.com [IPv6:2607:f8b0:4864:20::742]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C3ECEC06174E; Thu, 18 Jun 2020 10:53:26 -0700 (PDT) Received: by mail-qk1-x742.google.com with SMTP id l17so6395327qki.9; Thu, 18 Jun 2020 10:53:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Yq8e8gIvbXqQ4rRwZVqh8S7lLdpmKVcmmToQa1hhZLM=; b=iLqGHLCu1HSxVWeIze62d+6an/8ebsIyxu6pAjAvPkKier+IEupWQiTTEPfT0icPDt 0629C3+2Qhqw2DfGxJzfMrG6muZ847OqqOvQMbT7TTSLApMKBazVATc00AV56tFDuTrf axkvaRqNRE3hWBhPm6rFdJlfb1CMv6C0FlzOG5+dixFG3ZXEG+4kHufIPz+Bb+OWFZcB OM5fyrtKWa+P1mMvE3ocqdzCEZxSxiNYJ9om0n8urUufdi6GMR1A20pl6+ykF31CWB8R 5QZ6umQFtTBaaF3nDRifjoHRriMbXVf6USYVruxKg35nbWKQnffQuSTkIqCx2N/hQFTL kKwA== 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=Yq8e8gIvbXqQ4rRwZVqh8S7lLdpmKVcmmToQa1hhZLM=; b=OgMf7lfdusbvyNlYwa71eF0BdJqlNBpnmTUlgPF0skaH1lKvVarW1AH6S4+TM/lj0s STIEpwNX/gPZ/eg7EzO0hrMK8Z23fHOLCw2tTpPDKf+HxOgyleRd2/WhJODPpjFEhXAO NhQhDyVRmxP6w9az4KOz6Qdf9JoFjhE+Alpwc3xDwo4LJwP0VPllK1/gCbXVQS6XvNV5 nn0SKPiQng2u7ApGhYc0U9xRmm1DGlxdwm+iOlmrUhhIWoHgrqa0vHjTfgTrSWo8xmDH TcfuFoUNfL4c4uSmpbTtFDzBUUWfZQJB5akhvpsHAhb7ufhOFSbiKDz3jG5Px3vWSrAr toHQ== X-Gm-Message-State: AOAM533yo4Vznl1dLoOVltAYyY6n/OlEBQr9XQ6KoLGHT7ihmMgp2meJ 4fdVtJYmqo1Z5KmMigyhwoVk+qnYzzwV4zATy4c= X-Google-Smtp-Source: ABdhPJwa9Z+FAHeFhMeDqZXOdNTHTULvsTCmEKB9jkpgSopMTOCiTNP6aEV82wLvjpoLM0nssiEXm80iJfO48kHYWhw= X-Received: by 2002:a37:a89:: with SMTP id 131mr41442qkk.92.1592502805937; Thu, 18 Jun 2020 10:53:25 -0700 (PDT) MIME-Version: 1.0 References: <20200617161832.1438371-1-andriin@fb.com> <20200617161832.1438371-9-andriin@fb.com> In-Reply-To: From: Andrii Nakryiko Date: Thu, 18 Jun 2020 10:53:14 -0700 Message-ID: Subject: Re: [PATCH bpf-next 8/9] tools/bpftool: show info for processes holding BPF map/prog/link/btf FDs To: Quentin Monnet Cc: Andrii Nakryiko , bpf , Networking , Alexei Starovoitov , Daniel Borkmann , Kernel Team , Hao Luo , Arnaldo Carvalho de Melo , Song Liu Content-Type: text/plain; charset="UTF-8" Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Thu, Jun 18, 2020 at 12:51 AM Quentin Monnet wrote: > > 2020-06-17 23:01 UTC-0700 ~ Andrii Nakryiko > > On Wed, Jun 17, 2020 at 5:24 PM Quentin Monnet wrote: > >> > >> 2020-06-17 09:18 UTC-0700 ~ Andrii Nakryiko > >>> Add bpf_iter-based way to find all the processes that hold open FDs against > >>> BPF object (map, prog, link, btf). bpftool always attempts to discover this, > >>> but will silently give up if kernel doesn't yet support bpf_iter BPF programs. > >>> Process name and PID are emitted for each process (task group). > >>> > >>> Sample output for each of 4 BPF objects: > >>> > >>> $ sudo ./bpftool prog show > >>> 2694: cgroup_device tag 8c42dee26e8cd4c2 gpl > >>> loaded_at 2020-06-16T15:34:32-0700 uid 0 > >>> xlated 648B jited 409B memlock 4096B > >>> pids systemd(1) > >>> 2907: cgroup_skb name egress tag 9ad187367cf2b9e8 gpl > >>> loaded_at 2020-06-16T18:06:54-0700 uid 0 > >>> xlated 48B jited 59B memlock 4096B map_ids 2436 > >>> btf_id 1202 > >>> pids test_progs(2238417), test_progs(2238445) > >>> > >>> $ sudo ./bpftool map show > >>> 2436: array name test_cgr.bss flags 0x400 > >>> key 4B value 8B max_entries 1 memlock 8192B > >>> btf_id 1202 > >>> pids test_progs(2238417), test_progs(2238445) > >>> 2445: array name pid_iter.rodata flags 0x480 > >>> key 4B value 4B max_entries 1 memlock 8192B > >>> btf_id 1214 frozen > >>> pids bpftool(2239612) > >>> > >>> $ sudo ./bpftool link show > >>> 61: cgroup prog 2908 > >>> cgroup_id 375301 attach_type egress > >>> pids test_progs(2238417), test_progs(2238445) > >>> 62: cgroup prog 2908 > >>> cgroup_id 375344 attach_type egress > >>> pids test_progs(2238417), test_progs(2238445) > >>> > >>> $ sudo ./bpftool btf show > >>> 1202: size 1527B prog_ids 2908,2907 map_ids 2436 > >>> pids test_progs(2238417), test_progs(2238445) > >>> 1242: size 34684B > >>> pids bpftool(2258892) > >>> > >>> Signed-off-by: Andrii Nakryiko > >>> --- > >> > >> [...] > >> > >>> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c > >>> new file mode 100644 > >>> index 000000000000..3474a91743ff > >>> --- /dev/null > >>> +++ b/tools/bpf/bpftool/pids.c > >>> @@ -0,0 +1,229 @@ > >> > >> [...] > >> > >>> +int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type) > >>> +{ > >>> + char buf[4096]; > >>> + struct pid_iter_bpf *skel; > >>> + struct pid_iter_entry *e; > >>> + int err, ret, fd = -1, i; > >>> + libbpf_print_fn_t default_print; > >>> + > >>> + hash_init(table->table); > >>> + set_max_rlimit(); > >>> + > >>> + skel = pid_iter_bpf__open(); > >>> + if (!skel) { > >>> + p_err("failed to open PID iterator skeleton"); > >>> + return -1; > >>> + } > >>> + > >>> + skel->rodata->obj_type = type; > >>> + > >>> + /* we don't want output polluted with libbpf errors if bpf_iter is not > >>> + * supported > >>> + */ > >>> + default_print = libbpf_set_print(libbpf_print_none); > >>> + err = pid_iter_bpf__load(skel); > >>> + libbpf_set_print(default_print); > >>> + if (err) { > >>> + /* too bad, kernel doesn't support BPF iterators yet */ > >>> + err = 0; > >>> + goto out; > >>> + } > >>> + err = pid_iter_bpf__attach(skel); > >>> + if (err) { > >>> + /* if we loaded above successfully, attach has to succeed */ > >>> + p_err("failed to attach PID iterator: %d", err); > >> > >> Nit: What about using strerror(err) for the error messages, here and > >> below? It's easier to read than an integer value. > > > > I'm actually against it. Just a pure string message for error is often > > quite confusing. It's an extra step, and sometimes quite a quest in > > itself, to find what's the integer value of errno it was, just to try > > to understand what kind of error it actually is. So I certainly prefer > > having integer value, optionally with a string error message. > > > > But that's too much hassle for this "shouldn't happen" type of errors. > > If they happen, the user is unlikely to infer anything useful and fix > > the problem by themselves. They will most probably have to ask on the > > mailing list and paste error messages verbatim and let people like me > > and you try to guess what's going on. In such cases, having an errno > > number is much more helpful. > > Ok, fair enough. > > >>> + goto out; > >>> + } > >>> + > >>> + fd = bpf_iter_create(bpf_link__fd(skel->links.iter)); > >>> + if (fd < 0) { > >>> + err = -errno; > >>> + p_err("failed to create PID iterator session: %d", err); > >>> + goto out; > >>> + } > >>> + > >>> + while (true) { > >>> + ret = read(fd, buf, sizeof(buf)); > >>> + if (ret < 0) { > >>> + err = -errno; > >>> + p_err("failed to read PID iterator output: %d", err); > >>> + goto out; > >>> + } > >>> + if (ret == 0) > >>> + break; > >>> + if (ret % sizeof(*e)) { > >>> + err = -EINVAL; > >>> + p_err("invalid PID iterator output format"); > >>> + goto out; > >>> + } > >>> + ret /= sizeof(*e); > >>> + > >>> + e = (void *)buf; > >>> + for (i = 0; i < ret; i++, e++) { > >>> + add_ref(table, e); > >>> + } > >>> + } > >>> + err = 0; > >>> +out: > >>> + if (fd >= 0) > >>> + close(fd); > >>> + pid_iter_bpf__destroy(skel); > >>> + return err; > >>> +} > >> > >> [...] > >> > >>> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c > >>> new file mode 100644 > >>> index 000000000000..f560e48add07 > >>> --- /dev/null > >>> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c > >>> @@ -0,0 +1,80 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >> > >> This would make it the only file not dual-licensed GPL/BSD in bpftool. > >> We've had issues with that before [0], although linking to libbfd is no > >> more a hard requirement. But I see you used a dual-license in the > >> corresponding header file pid_iter.h, so is the single license > >> intentional here? Or would you consider GPL/BSD? > >> > > > > The other BPF program (skeleton/profiler.bpf.c) is also GPL-2.0, we > > should probably switch both. > > Oh I missed that one :(. Yeah, if this is possible, that would be great! > > >> [0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=896165#38 > >> > >>> +// Copyright (c) 2020 Facebook > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include "pid_iter.h" > >> > >> [...] > >> > >>> + > >>> +char LICENSE[] SEC("license") = "GPL"; > > > > I wonder if leaving this as GPL would be ok, if the source code itself > > is dual GPL/BSD? > > If the concern is to pass the verifier, it accepts a handful of > different strings (see include/linux/license.h), one of which is "Dual > BSD/GPL" and should probably be used in that case. Or did you have > something else in mind? Oh, awesome, wasn't aware of this. I'll use "Dual BSD/GPL" then.