Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Roman Gushchin]

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 <guro@xxxxxx>
> 
> 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 <guro@xxxxxx>
> > + */
> > +
> > +#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!