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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

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@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 <guro@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:

# 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!