lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 16 Mar 2020 11:54:28 +0000
From:   Quentin Monnet <quentin@...valent.com>
To:     Martin KaFai Lau <kafai@...com>, bpf@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, kernel-team@...com,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 4/4] bpftool: Add struct_ops support

2020-03-15 17:56 UTC-0700 ~ Martin KaFai Lau <kafai@...com>
> This patch adds struct_ops support to the bpftool.

[...]

> Signed-off-by: Martin KaFai Lau <kafai@...com>
> ---
>  .../Documentation/bpftool-struct_ops.rst      | 106 ++++
>  tools/bpf/bpftool/bash-completion/bpftool     |  28 +
>  tools/bpf/bpftool/main.c                      |   3 +-
>  tools/bpf/bpftool/main.h                      |   1 +
>  tools/bpf/bpftool/struct_ops.c                | 595 ++++++++++++++++++
>  5 files changed, 732 insertions(+), 1 deletion(-)
>  create mode 100644 tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
>  create mode 100644 tools/bpf/bpftool/struct_ops.c
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
> new file mode 100644
> index 000000000000..27aae5bc632e
> --- /dev/null
> +++ b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
> @@ -0,0 +1,106 @@
> +==================
> +bpftool-struct_ops
> +==================
> +-------------------------------------------------------------------------------
> +tool to register/unregister/introspect BPF struct_ops
> +-------------------------------------------------------------------------------
> +
> +:Manual section: 8
> +
> +SYNOPSIS
> +========
> +
> +	**bpftool** [*OPTIONS*] **struct_ops** *COMMAND*
> +
> +	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] }
> +
> +	*COMMANDS* :=
> +	{ **show** | **list** | **dump** | **register** | **unregister** | **help** }
> +
> +STRUCT_OPS COMMANDS
> +===================
> +
> +|	**bpftool** **struct_ops { show | list }** [*STRUCT_OPS_MAP*]
> +|	**bpftool** **struct_ops dump** [*STRUCT_OPS_MAP*]
> +|	**bpftool** **struct_ops register** *OBJ*
> +|	**bpftool** **struct_ops unregister** *STRUCT_OPS_MAP*
> +|	**bpftool** **struct_ops help**
> +|
> +|	*STRUCT_OPS_MAP* := { **id** *STRUCT_OPS_MAP_ID* | **name** *STRUCT_OPS_MAP_NAME* }
> +|	*OBJ* := /a/file/of/bpf_struct_ops.o
> +
> +
> +DESCRIPTION
> +===========
> +	**bpftool struct_ops { show | list }** [*STRUCT_OPS_MAP*]
> +		  Show brief information about the struct_ops in the system.
> +		  If *STRUCT_OPS_MAP* is specified, it shows information only
> +		  for the given struct_ops.  Otherwise, it lists all struct_ops
> +		  currently exists in the system.

Typo: s/exists/existing/

> +
> +		  Output will start with struct_ops map ID, followed by its map
> +		  name and its struct_ops's kernel type.
> +
> +	**bpftool struct_ops dump** [*STRUCT_OPS_MAP*]
> +		  Dump details information about the struct_ops in the system.
> +		  If *STRUCT_OPS_MAP* is specified, it dumps information only
> +		  for the given struct_ops.  Otherwise, it dumps all struct_ops
> +		  currently exists in the system.

Same here.

> +
> +	**bpftool struct_ops register** *OBJ*
> +		  Register bpf struct_ops from *OBJ*.  All struct_ops under
> +		  the ELF section ".struct_ops" will be registered to
> +		  its kernel subsystem.
> +
> +	**bpftool struct_ops unregister**  *STRUCT_OPS_MAP*
> +		  Unregister the *STRUCT_OPS_MAP* from the kernel subsystem.
> +
> +	**bpftool struct_ops help**
> +		  Print short help message.
> +
> +OPTIONS
> +=======
> +	-h, --help
> +		  Print short generic help message (similar to **bpftool help**).
> +
> +	-V, --version
> +		  Print version number (similar to **bpftool version**).
> +
> +	-j, --json
> +		  Generate JSON output. For commands that cannot produce JSON, this
> +		  option has no effect.
> +
> +	-p, --pretty
> +		  Generate human-readable JSON output. Implies **-j**.
> +
> +	-d, --debug
> +		  Print all logs available, even debug-level information. This
> +		  includes logs from libbpf as well as from the verifier, when
> +		  attempting to load programs.
> +
> +EXAMPLES
> +========
> +**# bpftool struct_ops show**
> +
> +::
> +
> +    100: dctcp           tcp_congestion_ops
> +    105: cubic           tcp_congestion_ops
> +
> +**# bpftool struct_ops unregister id 105**
> +
> +::
> +
> +   Unregistered tcp_congestion_ops cubic id 105
> +
> +**# bpftool struct_ops register bpf_cubic.o**
> +
> +::
> +
> +   Registered tcp_congestion_ops cubic id 110
> +
> +
> +SEE ALSO
> +========
> +	**bpftool-map**\ (8)
> +	**bpftool-prog**\ (8)

Other man pages link to all available bpftool-* pages. If you do not
want to do that, could you at least link to bpf(2) and bpftool(8) please?

[...]

> diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
> new file mode 100644
> index 000000000000..ba145e3d0d5d
> --- /dev/null
> +++ b/tools/bpf/bpftool/struct_ops.c
> @@ -0,0 +1,595 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/* Copyright (C) 2020 Facebook */
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <errno.h>
> +
> +#include <linux/err.h>

Nit: line break here, please

> +#include <bpf/libbpf.h>
> +#include <bpf/bpf.h>
> +#include <bpf/btf.h>

Nit again: Could you please have the includes in each block ordered
alphabetically?

> +
> +#include "json_writer.h"
> +#include "main.h"
> +
> +#define STRUCT_OPS_VALUE_PREFIX "bpf_struct_ops_"
> +

[...]

> +static struct bpf_map_info *map_info_alloc(__u32 *alloc_len)
> +{
> +	struct bpf_map_info *info;
> +
> +	if (get_map_info_type_id() < 0)
> +		return NULL;
> +
> +	info = calloc(1, map_info_alloc_len);
> +	if (!info)
> +		p_err("mem alloc failed");
> +	else
> +		*alloc_len = map_info_alloc_len;
> +
> +	return info;
> +}
> +
> +/* It iterates all struct_ops maps of the system.
> + * It returns the fd in "*res_fd" and map_info in "*info".
> + * In the very first iteration, info->id should be 0.
> + * An optional map "*name" filter can be specified.
> + * The filter can be made more flexibile in the future.

Typo: flexible

> + * e.g. filter by kernel-struct-ops-name, regex-name, glob-name, ...etc.
> + *
> + * Return value:
> + *     1: A struct_ops map found.  It is returned in "*res_fd" and "*info".
> + *	  The caller can continue to call get_next in the future.
> + *     0: No struct_ops map is returned.
> + *        All struct_ops map has been found.
> + *    -1: Error and the caller should abort the iteration.
> + */
> +static int get_next_struct_ops_map(const char *name, int *res_fd,
> +				   struct bpf_map_info *info, __u32 info_len)
> +{
> +	__u32 id = info->id;
> +	int err, fd;
> +
> +	while (true) {
> +		err = bpf_map_get_next_id(id, &id);
> +		if (err) {
> +			if (errno == ENOENT)
> +				return 0;
> +			p_err("can't get next map %s", strerror(errno));

Nit: Add a colon before "%s"?

> +			return -1;
> +		}
> +
> +		fd = bpf_map_get_fd_by_id(id);
> +		if (fd < 0) {
> +			if (errno == ENOENT)
> +				continue;
> +			p_err("can't get map by id (%u): %s",
> +			      id, strerror(errno));
> +			return -1;
> +		}
> +
> +		err = bpf_obj_get_info_by_fd(fd, info, &info_len);
> +		if (err) {
> +			p_err("can't get map info: %s", strerror(errno));
> +			close(fd);
> +			return -1;
> +		}
> +
> +		if (info->type == BPF_MAP_TYPE_STRUCT_OPS &&
> +		    (!name || !strcmp(name, info->name))) {
> +			*res_fd = fd;
> +			return 1;
> +		}
> +		close(fd);
> +	}
> +}

[...]

> +static int do_unregister(int argc, char **argv)
> +{
> +	const char *search_type, *search_term;
> +	struct res res;
> +
> +	if (argc != 2)
> +		usage();

Or you could reuse the macros in main.h, for more consistency with other
subcommands:

	if (!REQ_ARGS(2))
		return -1;

> +
> +	search_type = argv[0];
> +	search_term = argv[1];

	search_type = GET_ARG();
	search_term = GET_ARG();

> +
> +	res = do_work_on_struct_ops(search_type, search_term,
> +				    __do_unregister, NULL, NULL);
> +
> +	return cmd_retval(&res, true);
> +}
> +
> +static int do_register(int argc, char **argv)
> +{
> +	const struct bpf_map_def *def;
> +	struct bpf_map_info info = {};
> +	__u32 info_len = sizeof(info);
> +	int nr_errs = 0, nr_maps = 0;
> +	struct bpf_object *obj;
> +	struct bpf_link *link;
> +	struct bpf_map *map;
> +	const char *file;
> +
> +	if (argc != 1)
> +		usage();

(Same remark here.)

> +
> +	file = argv[0];
> +
> +	obj = bpf_object__open(file);
> +	if (IS_ERR_OR_NULL(obj))
> +		return -1;

[...]

Powered by blists - more mailing lists