On Thu, Dec 07, 2017 at 02:23:06PM -0800, Jakub Kicinski wrote:
> 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 <g...@fb.com>
>
> 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 <g...@fb.com>
> > + */
> > +
> > +#include <fcntl.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +
> > +#include <bpf.h>
> > +
> > +#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:
Fixed in v3.
>
> # 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.
Not sure about this case, as allow_multi and allow_override have
a common "allow_" prefix, so it might be confusing.
>
> 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!
Sure, will do separately.
> Otherwise looks really nice, thanks for working on it!
Thank you for reviewing!
