From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752426AbdLGWXN (ORCPT ); Thu, 7 Dec 2017 17:23:13 -0500 Received: from mail-qt0-f194.google.com ([209.85.216.194]:47029 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752257AbdLGWXL (ORCPT ); Thu, 7 Dec 2017 17:23:11 -0500 X-Google-Smtp-Source: AGs4zMZc4JxHaQrk5rH3nzlkP7IAieXKzQN+uWwzFwEi9WO1HCA1Y+PS7XbpXG7joowIMsWPgl9iCw== Date: Thu, 7 Dec 2017 14:23:06 -0800 From: Jakub Kicinski To: Roman Gushchin Cc: , , , , , , Quentin Monnet , David Ahern Subject: Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations Message-ID: <20171207142306.285be14c@cakuba.netronome.com> In-Reply-To: <20171207183909.16240-5-guro@fb.com> References: <20171207183909.16240-1-guro@fb.com> <20171207183909.16240-5-guro@fb.com> Organization: Netronome Systems, Ltd. MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote: > This patch adds basic cgroup bpf operations to bpftool: > cgroup list, attach and detach commands. > > Usage is described in the corresponding man pages, > and examples are provided. > > Syntax: > $ bpftool cgroup list CGROUP > $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS] > $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG > > Signed-off-by: Roman Gushchin Looks good, a few very minor nits/questions below. > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c > new file mode 100644 > index 000000000000..88d67f74313f > --- /dev/null > +++ b/tools/bpf/bpftool/cgroup.c > @@ -0,0 +1,305 @@ > +/* > + * Copyright (C) 2017 Facebook > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + * > + * Author: Roman Gushchin > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include "main.h" > + > +static const char * const attach_type_strings[] = { > + [BPF_CGROUP_INET_INGRESS] = "ingress", > + [BPF_CGROUP_INET_EGRESS] = "egress", > + [BPF_CGROUP_INET_SOCK_CREATE] = "sock_create", > + [BPF_CGROUP_SOCK_OPS] = "sock_ops", > + [BPF_CGROUP_DEVICE] = "device", > + [__MAX_BPF_ATTACH_TYPE] = NULL, > +}; > + > +static enum bpf_attach_type parse_attach_type(const char *str) > +{ > + enum bpf_attach_type type; > + > + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { > + if (attach_type_strings[type] && > + strcmp(str, attach_type_strings[type]) == 0) Here and for matching flags you use straight strcmp(), not our locally defined is_prefix(), is this intentional? is_prefix() allows abbreviations, like in iproute2 commands. E.g. this would work: # bpftool cg att /sys/fs/cgroup/test.slice/ dev id 1 allow_multi > + return type; > + } > + > + return __MAX_BPF_ATTACH_TYPE; > +} > +static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) > +{ > + __u32 attach_flags; > + __u32 prog_ids[1024] = {0}; > + __u32 prog_cnt, iter; > + char *attach_flags_str; > + int ret; nit: could you reorder the variables so they're listed longest to shortest (reverse christmas tree)? > + prog_cnt = ARRAY_SIZE(prog_ids); > + ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids, > + &prog_cnt); > + if (ret) > + return ret; > + > + if (prog_cnt == 0) > + return 0; > + > + switch (attach_flags) { > + case BPF_F_ALLOW_MULTI: > + attach_flags_str = "allow_multi"; > + break; > + case BPF_F_ALLOW_OVERRIDE: > + attach_flags_str = "allow_override"; > + break; > + case 0: > + attach_flags_str = ""; > + break; > + default: > + attach_flags_str = "unknown"; nit: would it make sense to perhaps print flags in hex format in this case? > + } > + > + for (iter = 0; iter < prog_cnt; iter++) > + list_bpf_prog(prog_ids[iter], attach_type_strings[type], > + attach_flags_str); > + > + return 0; > +} > +static int do_attach(int argc, char **argv) > +{ > + int cgroup_fd, prog_fd; > + enum bpf_attach_type attach_type; > + int attach_flags = 0; > + int i; > + int ret = -1; > + > + if (argc < 4) { > + p_err("too few parameters for cgroup attach\n"); > + goto exit; > + } > + > + cgroup_fd = open(argv[0], O_RDONLY); > + if (cgroup_fd < 0) { > + p_err("can't open cgroup %s\n", argv[1]); > + goto exit; > + } > + > + attach_type = parse_attach_type(argv[1]); > + if (attach_type == __MAX_BPF_ATTACH_TYPE) { > + p_err("invalid attach type\n"); > + goto exit_cgroup; > + } > + > + argc -= 2; > + argv = &argv[2]; > + prog_fd = prog_parse_fd(&argc, &argv); > + if (prog_fd < 0) > + goto exit_cgroup; > + > + for (i = 0; i < argc; i++) { > + if (strcmp(argv[i], "allow_multi") == 0) { > + attach_flags |= BPF_F_ALLOW_MULTI; > + } else if (strcmp(argv[i], "allow_override") == 0) { > + attach_flags |= BPF_F_ALLOW_OVERRIDE; This is the other potential place for is_prefix() I referred to. That reminds me - would you care to also update Quentin's bash completions (tools/bpf/bpftool/bash-completion/bpftool)? They are _very_ nice to have in day to day use! > + } else { > + p_err("unknown option: %s\n", argv[i]); > + goto exit_cgroup; > + } > + } > + > + if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, attach_flags)) { > + p_err("failed to attach program"); > + goto exit_prog; > + } > + > + if (json_output) > + jsonw_null(json_wtr); > + > + ret = 0; > + > +exit_prog: > + close(prog_fd); > +exit_cgroup: > + close(cgroup_fd); > +exit: > + return ret; > +} Otherwise looks really nice, thanks for working on it!