mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Quentin Monnet <qmo@kernel.org>
To: Tao Chen <chen.dylane@linux.dev>,
	ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	martin.lau@linux.dev, eddyz87@gmail.com, song@kernel.org,
	yonghong.song@linux.dev, john.fastabend@gmail.com,
	kpsingh@kernel.org, sdf@fomichev.me, haoluo@google.com,
	jolsa@kernel.org, davem@davemloft.net, kuba@kernel.org,
	hawk@kernel.org
Cc: linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next 1/2] bpftool: Add bpf_token show
Date: Mon, 21 Jul 2025 17:23:50 +0100	[thread overview]
Message-ID: <6b0669fd-fef6-4f4e-b80d-512769e86938@kernel.org> (raw)
In-Reply-To: <20250720173310.1334483-1-chen.dylane@linux.dev>

Thanks a lot for this!


2025-07-21 01:33 UTC+0800 ~ Tao Chen <chen.dylane@linux.dev>
> Add `bpftool token show` command to get token info
> from bpf fs in /proc/mounts.
> 
> Example plain output for `token show`:
> token_info:
>         /sys/fs/bpf/token
> 
> allowed_cmds:
>         map_create          prog_load
> 
> allowed_maps:
> 
> allowed_progs:
>         kprobe
> 
> allowed_attachs:
>         xdp
> 
> Example json output for `token show`:
> {
>     "token_info": "/sys/fs/bpf/token",
>     "allowed_cmds": ["map_create","prog_load"
>     ],
>     "allowed_maps":


This is not valid JSON. You're missing a value for "allowed_maps" (here
it should likely be an empty array), and the comma:

	"allowed_maps": [],


>     "allowed_progs": ["kprobe"
>     ],
>     "allowed_attachs": ["xdp"
>     ]
> }
> 
> Signed-off-by: Tao Chen <chen.dylane@linux.dev>
> ---
>  tools/bpf/bpftool/main.c  |   3 +-
>  tools/bpf/bpftool/main.h  |   1 +
>  tools/bpf/bpftool/token.c | 229 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 232 insertions(+), 1 deletion(-)
>  create mode 100644 tools/bpf/bpftool/token.c
> 
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 2b7f2bd3a7d..0f1183b2ed0 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -61,7 +61,7 @@ static int do_help(int argc, char **argv)
>  		"       %s batch file FILE\n"
>  		"       %s version\n"
>  		"\n"
> -		"       OBJECT := { prog | map | link | cgroup | perf | net | feature | btf | gen | struct_ops | iter }\n"
> +		"       OBJECT := { prog | map | link | cgroup | perf | net | feature | btf | gen | struct_ops | iter | token }\n"
>  		"       " HELP_SPEC_OPTIONS " |\n"
>  		"                    {-V|--version} }\n"
>  		"",
> @@ -87,6 +87,7 @@ static const struct cmd commands[] = {
>  	{ "gen",	do_gen },
>  	{ "struct_ops",	do_struct_ops },
>  	{ "iter",	do_iter },
> +	{ "token",	do_token },
>  	{ "version",	do_version },
>  	{ 0 }
>  };
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 6db704fda5c..a2bb0714b3d 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -166,6 +166,7 @@ int do_tracelog(int argc, char **arg) __weak;
>  int do_feature(int argc, char **argv) __weak;
>  int do_struct_ops(int argc, char **argv) __weak;
>  int do_iter(int argc, char **argv) __weak;
> +int do_token(int argc, char **argv) __weak;
>  
>  int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
>  int prog_parse_fd(int *argc, char ***argv);
> diff --git a/tools/bpf/bpftool/token.c b/tools/bpf/bpftool/token.c
> new file mode 100644
> index 00000000000..2fcaff4f2ba
> --- /dev/null
> +++ b/tools/bpf/bpftool/token.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <mntent.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +#include "json_writer.h"
> +#include "main.h"
> +
> +#define MOUNTS_FILE "/proc/mounts"
> +
> +#define zclose(fd) do { if (fd >= 0) close(fd); fd = -1; } while (0)


Seems unused?


> +
> +static bool has_delegate_options(const char *mnt_ops)
> +{
> +	return strstr(mnt_ops, "delegate_cmds") != NULL ||
> +	       strstr(mnt_ops, "delegate_maps") != NULL ||
> +	       strstr(mnt_ops, "delegate_progs") != NULL ||
> +	       strstr(mnt_ops, "delegate_attachs") != NULL;
> +}
> +
> +static char *get_delegate_value(const char *opts, const char *key)
> +{
> +	char *token, *rest, *ret = NULL;
> +	char *opts_copy = strdup(opts);
> +
> +	if (!opts_copy)
> +		return NULL;
> +
> +	for (token = strtok_r(opts_copy, ",", &rest); token != NULL;
> +			token = strtok_r(NULL, ",", &rest)) {
> +		if (strncmp(token, key, strlen(key)) == 0 &&
> +				token[strlen(key)] == '=') {
> +			ret = token + strlen(key) + 1;
> +			break;
> +		}
> +	}
> +	free(opts_copy);
> +
> +	return ret;
> +}
> +
> +static void print_items_per_line(const char *input, int items_per_line)
> +{
> +	char *str, *rest;
> +	int cnt = 0;
> +	char *strs = strdup(input);
> +
> +	if (!strs)
> +		return;
> +
> +	for (str = strtok_r(strs, ":", &rest); str != NULL;
> +			str = strtok_r(NULL, ":", &rest)) {
> +		if (cnt % items_per_line == 0)
> +			printf("\n\t");
> +
> +		printf("%-20s", str);
> +		cnt++;
> +	}
> +
> +	free(strs);
> +}
> +
> +#define ITEMS_PER_LINE 4
> +static void show_token_info_plain(struct mntent *mntent)
> +{
> +	char *value;
> +
> +	printf("\ntoken_info:");
> +	printf("\n\t%s\n", mntent->mnt_dir);
> +
> +	printf("\nallowed_cmds:");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_cmds");
> +	if (value)
> +		print_items_per_line(value, ITEMS_PER_LINE);
> +	printf("\n");
> +
> +	printf("\nallowed_maps:");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_maps");
> +	if (value)
> +		print_items_per_line(value, ITEMS_PER_LINE);
> +	printf("\n");
> +
> +	printf("\nallowed_progs:");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_progs");
> +	if (value)
> +		print_items_per_line(value, ITEMS_PER_LINE);
> +	printf("\n");
> +
> +	printf("\nallowed_attachs:");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_attachs");
> +	if (value)
> +		print_items_per_line(value, ITEMS_PER_LINE);
> +	printf("\n");
> +}
> +
> +static void __json_array_str(const char *input)


Nit: Why the double underscore in the function name? Let's use a more
explicit name also, maybe something like "split_to_json_array"?


> +{
> +	char *str, *rest;
> +	char *strs = strdup(input);
> +
> +	if (!strs)
> +		return;
> +
> +	jsonw_start_array(json_wtr);
> +	for (str = strtok_r(strs, ":", &rest); str != NULL;
> +			str = strtok_r(NULL, ":", &rest)) {
> +		jsonw_string(json_wtr, str);
> +	}
> +	jsonw_end_array(json_wtr);
> +
> +	free(strs);
> +}
> +
> +static void show_token_info_json(struct mntent *mntent)
> +{
> +	char *value;
> +
> +	jsonw_start_object(json_wtr);
> +
> +	jsonw_string_field(json_wtr, "token_info", mntent->mnt_dir);
> +
> +	jsonw_name(json_wtr, "allowed_cmds");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_cmds");
> +	if (value)
> +		__json_array_str(value);


As mentioned above, you need to change __json_array_str() to print
something when you don't get a "value" here - just have it print an
empty array.


> +
> +	jsonw_name(json_wtr, "allowed_maps");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_maps");
> +	if (value)
> +		__json_array_str(value);
> +
> +	jsonw_name(json_wtr, "allowed_progs");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_progs");
> +	if (value)
> +		__json_array_str(value);
> +
> +	jsonw_name(json_wtr, "allowed_attachs");
> +	value = get_delegate_value(mntent->mnt_opts, "delegate_attachs");
> +	if (value)
> +		__json_array_str(value);
> +
> +	jsonw_end_object(json_wtr);
> +}
> +
> +static int __show_token_info(struct mntent *mntent)
> +{
> +
> +	if (json_output)
> +		show_token_info_json(mntent);
> +	else
> +		show_token_info_plain(mntent);
> +
> +	return 0;
> +}
> +
> +static int show_token_info(void)
> +{
> +	FILE *fp;
> +	struct mntent *ent;
> +	bool hit = false;
> +
> +	fp = setmntent(MOUNTS_FILE, "r");
> +	if (!fp) {
> +		p_err("Failed to open:%s", MOUNTS_FILE);


Missing space after the colon, in the error message.


> +		return -1;
> +	}
> +
> +	while ((ent = getmntent(fp)) != NULL) {
> +		if (strcmp(ent->mnt_type, "bpf") == 0) {


File common.c has:

		if (strncmp(mntent->mnt_type, "bpf", 3) != 0)
			continue;

Maybe do the same for consistency, and to avoid indenting too far right?


> +			if (has_delegate_options(ent->mnt_opts)) {
> +				hit = true;
> +				break;


Apologies, my knowledge of BPF tokens is limited. Can you have only one
token exposed through a bpffs at a time? Asking because I know you can
have several bpffs on your system, if each can have delegate options
then why stop after the first bpffs mount point you find?


> +			}
> +		}
> +	}
> +
> +	if (hit)
> +		__show_token_info(ent);


Maybe at least a p_info() message if you don't find anything to print?


> +	endmntent(fp);
> +
> +	return 0;
> +}
> +
> +static int do_show(int argc, char **argv)
> +{
> +	if (argc)
> +		return BAD_ARG();
> +
> +	return show_token_info();
> +}
> +
> +static int do_help(int argc, char **argv)
> +{
> +	if (json_output) {
> +		jsonw_null(json_wtr);
> +		return 0;
> +	}
> +
> +	fprintf(stderr,
> +		"Usage: %1$s %2$s { show | list }\n"
> +		"	%1$s %2$s help\n"
> +		"\n"
> +		"",
> +		bin_name, argv[-2]);
> +	return 0;
> +}
> +
> +static const struct cmd cmds[] = {
> +	{ "show",	do_show },
> +	{ "help",	do_help },
> +	{ "list",	do_show },


Nit: Can we have "help" coming third, below both "show" and "list" please?


> +	{ 0 }
> +};
> +
> +int do_token(int argc, char **argv)
> +{
> +	return cmd_select(cmds, argc, argv, do_help);
> +}


  parent reply	other threads:[~2025-07-21 16:23 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-20 17:33 Tao Chen
2025-07-20 17:33 ` [PATCH bpf-next 2/2] bpftool: Add bpftool-token manpage Tao Chen
2025-07-21 16:23   ` Quentin Monnet
2025-07-22  6:06     ` Tao Chen
2025-07-21 16:23 ` Quentin Monnet [this message]
2025-07-22  5:48   ` [PATCH bpf-next 1/2] bpftool: Add bpf_token show Tao Chen
2025-07-22  9:23     ` Quentin Monnet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6b0669fd-fef6-4f4e-b80d-512769e86938@kernel.org \
    --to=qmo@kernel.org \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=chen.dylane@linux.dev \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=eddyz87@gmail.com \
    --cc=haoluo@google.com \
    --cc=hawk@kernel.org \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.lau@linux.dev \
    --cc=netdev@vger.kernel.org \
    --cc=sdf@fomichev.me \
    --cc=song@kernel.org \
    --cc=yonghong.song@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®