[PATCH RFC bpf-next 0/3] BPF FS mount options parsing follow ups

[PATCH RFC bpf-next 0/3] BPF FS mount options parsing follow ups

[Andrii Nakryiko]

Original BPF token patch set ([0]) added delegate_xxx mount options which
supported only special "any" value and hexadecimal bitmask. This patch set
attempts to make specifying and inspecting these mount options more
human-friendly by supporting string constants matching corresponding bpf_cmd,
bpf_map_type, bpf_prog_type, and bpf_attach_type enumerators.

This is an RFC patch set and I've only converted bpf_cmd enum to be generated
through reusable mapper macro. If the consensus is that this approach is the
way to go, adding similar support for three remaining enums is just a matter
of mundane mechanical conversion in UAPI header. Kernel-side logic for all
delegate_xxx mount options is completely generic already as implemented in
patch #1.

  [0] https://patchwork.kernel.org/project/netdevbpf/list/?series=805707&state=*

Andrii Nakryiko (3):
  bpf: add mapper macro for bpf_cmd enum
  bpf: extend parsing logic for BPF FS delegate_cmds mount option
  selftests/bpf: utilize string values for delegate_xxx mount options

 include/uapi/linux/bpf.h                      |  81 +++++------
 kernel/bpf/inode.c                            | 127 +++++++++++++-----
 tools/include/uapi/linux/bpf.h                |  81 +++++------
 .../testing/selftests/bpf/prog_tests/token.c  |  43 +++---
 4 files changed, 208 insertions(+), 124 deletions(-)

-- 
2.34.1

[PATCH RFC bpf-next 1/3] bpf: add mapper macro for bpf_cmd enum

[Andrii Nakryiko]

Use similar approach to enum bpf_func_id and generate enumerators using
a macro with macro callback. This approach allows to generate derivative
tables for string-based lookups and whatnot. In this particular case,
this mapper macro will be used for parsing BPF FS delegate_cmds mount
option and their human-readable output format in mount info.

Validated no regressions using before/after BTF through
`bpftool btf dump <file> format c` command.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/uapi/linux/bpf.h       | 81 ++++++++++++++++++----------------
 tools/include/uapi/linux/bpf.h | 81 ++++++++++++++++++----------------
 2 files changed, 86 insertions(+), 76 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e0545201b55f..d05ea24ace3f 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -893,47 +893,52 @@ union bpf_iter_link_info {
  *	to the object have been closed and no references remain pinned to the
  *	filesystem or attached (for example, bound to a program or device).
  */
+#define __BPF_CMD_MAPPER(FN, ctx...)					\
+	FN(BPF_MAP_CREATE, 0)						\
+	FN(BPF_MAP_LOOKUP_ELEM, 1)					\
+	FN(BPF_MAP_UPDATE_ELEM, 2)					\
+	FN(BPF_MAP_DELETE_ELEM, 3)					\
+	FN(BPF_MAP_GET_NEXT_KEY, 4)					\
+	FN(BPF_PROG_LOAD, 5)						\
+	FN(BPF_OBJ_PIN, 6)						\
+	FN(BPF_OBJ_GET, 7)						\
+	FN(BPF_PROG_ATTACH, 8)						\
+	FN(BPF_PROG_DETACH, 9)						\
+	FN(BPF_PROG_TEST_RUN, 10)					\
+	FN(BPF_PROG_RUN, 10) /* alias for BPF_PROG_TEST_RUN */		\
+	FN(BPF_PROG_GET_NEXT_ID, 11)					\
+	FN(BPF_MAP_GET_NEXT_ID, 12)					\
+	FN(BPF_PROG_GET_FD_BY_ID, 13)					\
+	FN(BPF_MAP_GET_FD_BY_ID, 14)					\
+	FN(BPF_OBJ_GET_INFO_BY_FD, 15)					\
+	FN(BPF_PROG_QUERY, 16)						\
+	FN(BPF_RAW_TRACEPOINT_OPEN, 17)					\
+	FN(BPF_BTF_LOAD, 18)						\
+	FN(BPF_BTF_GET_FD_BY_ID, 19)					\
+	FN(BPF_TASK_FD_QUERY, 20)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_ELEM, 21)				\
+	FN(BPF_MAP_FREEZE, 22)						\
+	FN(BPF_BTF_GET_NEXT_ID, 23)					\
+	FN(BPF_MAP_LOOKUP_BATCH, 24)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_BATCH, 25)				\
+	FN(BPF_MAP_UPDATE_BATCH, 26)					\
+	FN(BPF_MAP_DELETE_BATCH, 27)					\
+	FN(BPF_LINK_CREATE, 28)						\
+	FN(BPF_LINK_UPDATE, 29)						\
+	FN(BPF_LINK_GET_FD_BY_ID, 30)					\
+	FN(BPF_LINK_GET_NEXT_ID, 31)					\
+	FN(BPF_ENABLE_STATS, 32)					\
+	FN(BPF_ITER_CREATE, 33)						\
+	FN(BPF_LINK_DETACH, 34)						\
+	FN(BPF_PROG_BIND_MAP, 35)					\
+	FN(BPF_TOKEN_CREATE, 36)					\
+	/* */
+#define __BPF_CMD_FN(x, y) x = y,
 enum bpf_cmd {
-	BPF_MAP_CREATE,
-	BPF_MAP_LOOKUP_ELEM,
-	BPF_MAP_UPDATE_ELEM,
-	BPF_MAP_DELETE_ELEM,
-	BPF_MAP_GET_NEXT_KEY,
-	BPF_PROG_LOAD,
-	BPF_OBJ_PIN,
-	BPF_OBJ_GET,
-	BPF_PROG_ATTACH,
-	BPF_PROG_DETACH,
-	BPF_PROG_TEST_RUN,
-	BPF_PROG_RUN = BPF_PROG_TEST_RUN,
-	BPF_PROG_GET_NEXT_ID,
-	BPF_MAP_GET_NEXT_ID,
-	BPF_PROG_GET_FD_BY_ID,
-	BPF_MAP_GET_FD_BY_ID,
-	BPF_OBJ_GET_INFO_BY_FD,
-	BPF_PROG_QUERY,
-	BPF_RAW_TRACEPOINT_OPEN,
-	BPF_BTF_LOAD,
-	BPF_BTF_GET_FD_BY_ID,
-	BPF_TASK_FD_QUERY,
-	BPF_MAP_LOOKUP_AND_DELETE_ELEM,
-	BPF_MAP_FREEZE,
-	BPF_BTF_GET_NEXT_ID,
-	BPF_MAP_LOOKUP_BATCH,
-	BPF_MAP_LOOKUP_AND_DELETE_BATCH,
-	BPF_MAP_UPDATE_BATCH,
-	BPF_MAP_DELETE_BATCH,
-	BPF_LINK_CREATE,
-	BPF_LINK_UPDATE,
-	BPF_LINK_GET_FD_BY_ID,
-	BPF_LINK_GET_NEXT_ID,
-	BPF_ENABLE_STATS,
-	BPF_ITER_CREATE,
-	BPF_LINK_DETACH,
-	BPF_PROG_BIND_MAP,
-	BPF_TOKEN_CREATE,
+	__BPF_CMD_MAPPER(__BPF_CMD_FN)
 	__MAX_BPF_CMD,
 };
+#undef __BPF_CMD_FN
 
 enum bpf_map_type {
 	BPF_MAP_TYPE_UNSPEC,
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e0545201b55f..d05ea24ace3f 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -893,47 +893,52 @@ union bpf_iter_link_info {
  *	to the object have been closed and no references remain pinned to the
  *	filesystem or attached (for example, bound to a program or device).
  */
+#define __BPF_CMD_MAPPER(FN, ctx...)					\
+	FN(BPF_MAP_CREATE, 0)						\
+	FN(BPF_MAP_LOOKUP_ELEM, 1)					\
+	FN(BPF_MAP_UPDATE_ELEM, 2)					\
+	FN(BPF_MAP_DELETE_ELEM, 3)					\
+	FN(BPF_MAP_GET_NEXT_KEY, 4)					\
+	FN(BPF_PROG_LOAD, 5)						\
+	FN(BPF_OBJ_PIN, 6)						\
+	FN(BPF_OBJ_GET, 7)						\
+	FN(BPF_PROG_ATTACH, 8)						\
+	FN(BPF_PROG_DETACH, 9)						\
+	FN(BPF_PROG_TEST_RUN, 10)					\
+	FN(BPF_PROG_RUN, 10) /* alias for BPF_PROG_TEST_RUN */		\
+	FN(BPF_PROG_GET_NEXT_ID, 11)					\
+	FN(BPF_MAP_GET_NEXT_ID, 12)					\
+	FN(BPF_PROG_GET_FD_BY_ID, 13)					\
+	FN(BPF_MAP_GET_FD_BY_ID, 14)					\
+	FN(BPF_OBJ_GET_INFO_BY_FD, 15)					\
+	FN(BPF_PROG_QUERY, 16)						\
+	FN(BPF_RAW_TRACEPOINT_OPEN, 17)					\
+	FN(BPF_BTF_LOAD, 18)						\
+	FN(BPF_BTF_GET_FD_BY_ID, 19)					\
+	FN(BPF_TASK_FD_QUERY, 20)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_ELEM, 21)				\
+	FN(BPF_MAP_FREEZE, 22)						\
+	FN(BPF_BTF_GET_NEXT_ID, 23)					\
+	FN(BPF_MAP_LOOKUP_BATCH, 24)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_BATCH, 25)				\
+	FN(BPF_MAP_UPDATE_BATCH, 26)					\
+	FN(BPF_MAP_DELETE_BATCH, 27)					\
+	FN(BPF_LINK_CREATE, 28)						\
+	FN(BPF_LINK_UPDATE, 29)						\
+	FN(BPF_LINK_GET_FD_BY_ID, 30)					\
+	FN(BPF_LINK_GET_NEXT_ID, 31)					\
+	FN(BPF_ENABLE_STATS, 32)					\
+	FN(BPF_ITER_CREATE, 33)						\
+	FN(BPF_LINK_DETACH, 34)						\
+	FN(BPF_PROG_BIND_MAP, 35)					\
+	FN(BPF_TOKEN_CREATE, 36)					\
+	/* */
+#define __BPF_CMD_FN(x, y) x = y,
 enum bpf_cmd {
-	BPF_MAP_CREATE,
-	BPF_MAP_LOOKUP_ELEM,
-	BPF_MAP_UPDATE_ELEM,
-	BPF_MAP_DELETE_ELEM,
-	BPF_MAP_GET_NEXT_KEY,
-	BPF_PROG_LOAD,
-	BPF_OBJ_PIN,
-	BPF_OBJ_GET,
-	BPF_PROG_ATTACH,
-	BPF_PROG_DETACH,
-	BPF_PROG_TEST_RUN,
-	BPF_PROG_RUN = BPF_PROG_TEST_RUN,
-	BPF_PROG_GET_NEXT_ID,
-	BPF_MAP_GET_NEXT_ID,
-	BPF_PROG_GET_FD_BY_ID,
-	BPF_MAP_GET_FD_BY_ID,
-	BPF_OBJ_GET_INFO_BY_FD,
-	BPF_PROG_QUERY,
-	BPF_RAW_TRACEPOINT_OPEN,
-	BPF_BTF_LOAD,
-	BPF_BTF_GET_FD_BY_ID,
-	BPF_TASK_FD_QUERY,
-	BPF_MAP_LOOKUP_AND_DELETE_ELEM,
-	BPF_MAP_FREEZE,
-	BPF_BTF_GET_NEXT_ID,
-	BPF_MAP_LOOKUP_BATCH,
-	BPF_MAP_LOOKUP_AND_DELETE_BATCH,
-	BPF_MAP_UPDATE_BATCH,
-	BPF_MAP_DELETE_BATCH,
-	BPF_LINK_CREATE,
-	BPF_LINK_UPDATE,
-	BPF_LINK_GET_FD_BY_ID,
-	BPF_LINK_GET_NEXT_ID,
-	BPF_ENABLE_STATS,
-	BPF_ITER_CREATE,
-	BPF_LINK_DETACH,
-	BPF_PROG_BIND_MAP,
-	BPF_TOKEN_CREATE,
+	__BPF_CMD_MAPPER(__BPF_CMD_FN)
 	__MAX_BPF_CMD,
 };
+#undef __BPF_CMD_FN
 
 enum bpf_map_type {
 	BPF_MAP_TYPE_UNSPEC,
-- 
2.34.1

[PATCH RFC bpf-next 2/3] bpf: extend parsing logic for BPF FS delegate_cmds mount option

[Andrii Nakryiko]

Besides already supported special "any" value and hex bit mask, support
string-based parsing of enum bpf_cmd values based on exact enumerator
names. We use __BPF_CMD_MAPPER macro to generate a lookup table. So
"BPF_PROG_LOAD" and "BPF_MAP_CREATE" are valid values to specify for
delegate_cmds options.

A bunch of code changes are setting up generic routines which will make
similar support for delegate_maps, delegate_progs, and delegate_attachs
mount options trivial to add once we have similar mapper macros for
respective enums.

Besides supporting string values, we also support multiple values
specified at the same time, using colon (':') separator.

There are corresponding changes on bpf_show_options side to use known
values to print them in human-readable format, falling back to hex mask
printing, if there are any unrecognized bits (which shouldn't happen for
delegate_cmds, but is necessary for the same routing to be able to
handle other delegate_xxx options).

Example below shows various ways to specify delegate_cmds options
through mount command and how mount options are printed back:

  $ sudo mkdir -p /sys/fs/bpf/token
  $ sudo mount -t bpf bpffs /sys/fs/bpf/token \
               -o delegate_cmds=BPF_PROG_LOAD \
               -o delegate_cmds=BPF_MAP_CREATE \
               -o delegate_cmds=BPF_TOKEN_CREATE:BPF_BTF_LOAD:BPF_LINK_CREATE
  $ mount | grep token
  bpffs on /sys/fs/bpf/token type bpf (rw,relatime,delegate_cmds=BPF_MAP_CREATE:BPF_PROG_LOAD:BPF_BTF_LOAD:BPF_LINK_CREATE:BPF_TOKEN_CREATE)

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/inode.c | 127 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 96 insertions(+), 31 deletions(-)

diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index 5359a0929c35..20b2d170fc0b 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -595,6 +595,54 @@ struct bpf_prog *bpf_prog_get_type_path(const char *name, enum bpf_prog_type typ
 }
 EXPORT_SYMBOL(bpf_prog_get_type_path);
 
+#define __BPF_KV_FN(name, val) { #name, val },
+static const struct constant_table cmd_kvs[] = {
+	__BPF_CMD_MAPPER(__BPF_KV_FN)
+	{}
+};
+static const struct constant_table map_kvs[] = {
+	{}
+};
+static const struct constant_table prog_kvs[] = {
+	{}
+};
+static const struct constant_table attach_kvs[] = {
+	{}
+};
+#undef __BPF_KV_FN
+
+static void seq_print_delegate_opts(struct seq_file *m,
+				    const char *opt_name,
+				    const struct constant_table *tbl,
+				    u64 delegate_msk, u64 any_msk)
+{
+	bool first = true;
+	u64 msk;
+	int i;
+
+	delegate_msk &= any_msk; /* clear unknown bits */
+
+	if (delegate_msk == 0)
+		return;
+
+	if (delegate_msk == any_msk) {
+		seq_printf(m, ",%s=any", opt_name);
+		return;
+	}
+
+	seq_printf(m, ",%s", opt_name);
+	for (i = 0; cmd_kvs[i].name; i++) {
+		msk = 1ULL << cmd_kvs[i].value;
+		if (delegate_msk & msk) {
+			seq_printf(m, "%c%s", first ? '=' : ':', cmd_kvs[i].name);
+			delegate_msk &= ~msk;
+			first = false;
+		}
+	}
+	if (delegate_msk)
+		seq_printf(m, "%c0x%llx", first ? '=' : ':', delegate_msk);
+}
+
 /*
  * Display the mount options in /proc/mounts.
  */
@@ -608,28 +656,17 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root)
 		seq_printf(m, ",mode=%o", mode);
 
 	mask = (1ULL << __MAX_BPF_CMD) - 1;
-	if ((opts->delegate_cmds & mask) == mask)
-		seq_printf(m, ",delegate_cmds=any");
-	else if (opts->delegate_cmds)
-		seq_printf(m, ",delegate_cmds=0x%llx", opts->delegate_cmds);
+	seq_print_delegate_opts(m, "delegate_cmds", cmd_kvs, opts->delegate_cmds, mask);
 
 	mask = (1ULL << __MAX_BPF_MAP_TYPE) - 1;
-	if ((opts->delegate_maps & mask) == mask)
-		seq_printf(m, ",delegate_maps=any");
-	else if (opts->delegate_maps)
-		seq_printf(m, ",delegate_maps=0x%llx", opts->delegate_maps);
+	seq_print_delegate_opts(m, "delegate_maps", map_kvs, opts->delegate_maps, mask);
 
 	mask = (1ULL << __MAX_BPF_PROG_TYPE) - 1;
-	if ((opts->delegate_progs & mask) == mask)
-		seq_printf(m, ",delegate_progs=any");
-	else if (opts->delegate_progs)
-		seq_printf(m, ",delegate_progs=0x%llx", opts->delegate_progs);
+	seq_print_delegate_opts(m, "delegate_progs", prog_kvs, opts->delegate_progs, mask);
 
 	mask = (1ULL << __MAX_BPF_ATTACH_TYPE) - 1;
-	if ((opts->delegate_attachs & mask) == mask)
-		seq_printf(m, ",delegate_attachs=any");
-	else if (opts->delegate_attachs)
-		seq_printf(m, ",delegate_attachs=0x%llx", opts->delegate_attachs);
+	seq_print_delegate_opts(m, "delegate_attachs", attach_kvs, opts->delegate_attachs, mask);
+
 	return 0;
 }
 
@@ -673,7 +710,6 @@ static int bpf_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	struct bpf_mount_opts *opts = fc->s_fs_info;
 	struct fs_parse_result result;
 	int opt, err;
-	u64 msk;
 
 	opt = fs_parse(fc, bpf_fs_parameters, param, &result);
 	if (opt < 0) {
@@ -700,26 +736,55 @@ static int bpf_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	case OPT_DELEGATE_CMDS:
 	case OPT_DELEGATE_MAPS:
 	case OPT_DELEGATE_PROGS:
-	case OPT_DELEGATE_ATTACHS:
-		if (strcmp(param->string, "any") == 0) {
-			msk = ~0ULL;
-		} else {
-			err = kstrtou64(param->string, 0, &msk);
-			if (err)
-				return err;
+	case OPT_DELEGATE_ATTACHS: {
+		const struct constant_table *kvs;
+		u64 *delegate_msk, msk = 0;
+		char *p;
+		int val;
+
+		switch (opt) {
+		case OPT_DELEGATE_CMDS:
+			delegate_msk = &opts->delegate_cmds;
+			kvs = cmd_kvs;
+			break;
+		case OPT_DELEGATE_MAPS:
+			delegate_msk = &opts->delegate_maps;
+			kvs = map_kvs;
+			break;
+		case OPT_DELEGATE_PROGS:
+			delegate_msk = &opts->delegate_progs;
+			kvs = prog_kvs;
+			break;
+		case OPT_DELEGATE_ATTACHS:
+			delegate_msk = &opts->delegate_attachs;
+			kvs = attach_kvs;
+			break;
+		default:
+			return -EINVAL;
 		}
+
+		while ((p = strsep(&param->string, ":"))) {
+			if (strcmp(p, "any") == 0) {
+				msk |= ~0ULL;
+			} else if ((val = lookup_constant(kvs, p, -1)) >= 0) {
+				msk |= 1ULL << val;
+			} else {
+				err = kstrtou64(p, 0, &msk);
+				if (err)
+					return err;
+			}
+		}
+
 		/* Setting delegation mount options requires privileges */
 		if (msk && !capable(CAP_SYS_ADMIN))
 			return -EPERM;
-		switch (opt) {
-		case OPT_DELEGATE_CMDS: opts->delegate_cmds |= msk; break;
-		case OPT_DELEGATE_MAPS: opts->delegate_maps |= msk; break;
-		case OPT_DELEGATE_PROGS: opts->delegate_progs |= msk; break;
-		case OPT_DELEGATE_ATTACHS: opts->delegate_attachs |= msk; break;
-		default: return -EINVAL;
-		}
+
+		*delegate_msk |= msk;
 		break;
 	}
+	default:
+		/* ignore unknown mount options */
+	}
 
 	return 0;
 }
-- 
2.34.1

[PATCH RFC bpf-next 3/3] selftests/bpf: utilize string values for delegate_xxx mount options

[Andrii Nakryiko]

Use both hex-based and string-based way to specify delegate mount
options for BPF FS.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/token.c  | 43 +++++++++++--------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/token.c b/tools/testing/selftests/bpf/prog_tests/token.c
index dc03790c6272..ec59c81c54b5 100644
--- a/tools/testing/selftests/bpf/prog_tests/token.c
+++ b/tools/testing/selftests/bpf/prog_tests/token.c
@@ -55,14 +55,22 @@ static int restore_priv_caps(__u64 old_caps)
 	return cap_enable_effective(old_caps, NULL);
 }
 
-static int set_delegate_mask(int fs_fd, const char *key, __u64 mask)
+static int set_delegate_mask(int fs_fd, const char *key, __u64 mask, const char *mask_str)
 {
 	char buf[32];
 	int err;
 
-	snprintf(buf, sizeof(buf), "0x%llx", (unsigned long long)mask);
+	if (!mask_str) {
+		if (mask == ~0ULL) {
+			mask_str = "any";
+		} else {
+			snprintf(buf, sizeof(buf), "0x%llx", (unsigned long long)mask);
+			mask_str = buf;
+		}
+	}
+
 	err = sys_fsconfig(fs_fd, FSCONFIG_SET_STRING, key,
-			   mask == ~0ULL ? "any" : buf, 0);
+			   mask_str, 0);
 	if (err < 0)
 		err = -errno;
 	return err;
@@ -72,6 +80,7 @@ static int set_delegate_mask(int fs_fd, const char *key, __u64 mask)
 
 struct bpffs_opts {
 	__u64 cmds;
+	const char *cmds_str;
 	__u64 maps;
 	__u64 progs;
 	__u64 attachs;
@@ -93,16 +102,16 @@ static int materialize_bpffs_fd(int fs_fd, struct bpffs_opts *opts)
 	int mnt_fd, err;
 
 	/* set up token delegation mount options */
-	err = set_delegate_mask(fs_fd, "delegate_cmds", opts->cmds);
+	err = set_delegate_mask(fs_fd, "delegate_cmds", opts->cmds, opts->cmds_str);
 	if (!ASSERT_OK(err, "fs_cfg_cmds"))
 		return err;
-	err = set_delegate_mask(fs_fd, "delegate_maps", opts->maps);
+	err = set_delegate_mask(fs_fd, "delegate_maps", opts->maps, NULL);
 	if (!ASSERT_OK(err, "fs_cfg_maps"))
 		return err;
-	err = set_delegate_mask(fs_fd, "delegate_progs", opts->progs);
+	err = set_delegate_mask(fs_fd, "delegate_progs", opts->progs, NULL);
 	if (!ASSERT_OK(err, "fs_cfg_progs"))
 		return err;
-	err = set_delegate_mask(fs_fd, "delegate_attachs", opts->attachs);
+	err = set_delegate_mask(fs_fd, "delegate_attachs", opts->attachs, NULL);
 	if (!ASSERT_OK(err, "fs_cfg_attachs"))
 		return err;
 
@@ -284,13 +293,13 @@ static void child(int sock_fd, struct bpffs_opts *opts, child_callback_fn callba
 	}
 
 	/* ensure unprivileged child cannot set delegation options */
-	err = set_delegate_mask(fs_fd, "delegate_cmds", 0x1);
+	err = set_delegate_mask(fs_fd, "delegate_cmds", 0x1, NULL);
 	ASSERT_EQ(err, -EPERM, "delegate_cmd_eperm");
-	err = set_delegate_mask(fs_fd, "delegate_maps", 0x1);
+	err = set_delegate_mask(fs_fd, "delegate_maps", 0x1, NULL);
 	ASSERT_EQ(err, -EPERM, "delegate_maps_eperm");
-	err = set_delegate_mask(fs_fd, "delegate_progs", 0x1);
+	err = set_delegate_mask(fs_fd, "delegate_progs", 0x1, NULL);
 	ASSERT_EQ(err, -EPERM, "delegate_progs_eperm");
-	err = set_delegate_mask(fs_fd, "delegate_attachs", 0x1);
+	err = set_delegate_mask(fs_fd, "delegate_attachs", 0x1, NULL);
 	ASSERT_EQ(err, -EPERM, "delegate_attachs_eperm");
 
 	/* pass BPF FS context object to parent */
@@ -314,22 +323,22 @@ static void child(int sock_fd, struct bpffs_opts *opts, child_callback_fn callba
 	}
 
 	/* ensure unprivileged child cannot reconfigure to set delegation options */
-	err = set_delegate_mask(fs_fd, "delegate_cmds", ~0ULL);
+	err = set_delegate_mask(fs_fd, "delegate_cmds", 0, "any");
 	if (!ASSERT_EQ(err, -EPERM, "delegate_cmd_eperm_reconfig")) {
 		err = -EINVAL;
 		goto cleanup;
 	}
-	err = set_delegate_mask(fs_fd, "delegate_maps", ~0ULL);
+	err = set_delegate_mask(fs_fd, "delegate_maps", 0, "any");
 	if (!ASSERT_EQ(err, -EPERM, "delegate_maps_eperm_reconfig")) {
 		err = -EINVAL;
 		goto cleanup;
 	}
-	err = set_delegate_mask(fs_fd, "delegate_progs", ~0ULL);
+	err = set_delegate_mask(fs_fd, "delegate_progs", 0, "any");
 	if (!ASSERT_EQ(err, -EPERM, "delegate_progs_eperm_reconfig")) {
 		err = -EINVAL;
 		goto cleanup;
 	}
-	err = set_delegate_mask(fs_fd, "delegate_attachs", ~0ULL);
+	err = set_delegate_mask(fs_fd, "delegate_attachs", 0, "any");
 	if (!ASSERT_EQ(err, -EPERM, "delegate_attachs_eperm_reconfig")) {
 		err = -EINVAL;
 		goto cleanup;
@@ -647,7 +656,7 @@ void test_token(void)
 {
 	if (test__start_subtest("map_token")) {
 		struct bpffs_opts opts = {
-			.cmds = 1ULL << BPF_MAP_CREATE,
+			.cmds_str = "BPF_MAP_CREATE",
 			.maps = 1ULL << BPF_MAP_TYPE_STACK,
 		};
 
@@ -662,7 +671,7 @@ void test_token(void)
 	}
 	if (test__start_subtest("prog_token")) {
 		struct bpffs_opts opts = {
-			.cmds = 1ULL << BPF_PROG_LOAD,
+			.cmds_str = "BPF_PROG_LOAD",
 			.progs = 1ULL << BPF_PROG_TYPE_XDP,
 			.attachs = 1ULL << BPF_XDP,
 		};
-- 
2.34.1

Re: [PATCH RFC bpf-next 1/3] bpf: add mapper macro for bpf_cmd enum

[Alexei Starovoitov]

On Thu, Dec 7, 2023 at 2:28 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> +#define __BPF_CMD_MAPPER(FN, ctx...)                                   \
> +       FN(BPF_MAP_CREATE, 0)                                           \
> +       FN(BPF_MAP_LOOKUP_ELEM, 1)                                      \
> +       FN(BPF_MAP_UPDATE_ELEM, 2)                                      \
> +       FN(BPF_MAP_DELETE_ELEM, 3)                                      \
> +       FN(BPF_MAP_GET_NEXT_KEY, 4)                                     \

So macro conversion across 4 main enums in uapi/bpf.h
is just to do:
+static const struct constant_table cmd_kvs[] = {
+       __BPF_CMD_MAPPER(__BPF_KV_FN)
+       {}
+};

on the kernel side,
right?

While in libbpf we already hard code name to value in arrays:
prog_type_name[], map_type_name[]

which probably will remain as-is, since libbpf needs to be
built independently from the kernel.
(unless we will say that tools/uapi/bpf.h is part of libbpf,
which probably not a good way).

There are more pros than cons in this enum uglification,
but cons are definitely staring in the face.

Have you considered other options?
Like using vmlinix BTF for parsing bpffs delegation?
[14083] ENUM 'bpf_cmd' encoding=UNSIGNED size=4 vlen=39
        'BPF_MAP_CREATE' val=0
        'BPF_MAP_LOOKUP_ELEM' val=1
        'BPF_MAP_UPDATE_ELEM' val=2
        'BPF_MAP_DELETE_ELEM' val=3
        'BPF_MAP_GET_NEXT_KEY' val=4
        'BPF_PROG_LOAD' val=5

Names and values are available.
btf_find_by_name_kind(vmlinux_btf, "bpd_cmd", BTF_KIND_ENUM);
is fast enough.

I suspect you'll argue that you don't want to tie in
bpffs delegation parsing with BTF ;)

While I can preemptively answer that in the case vmlinux BTF
is not available it's fine not to parse names and rely on hex.

Re: [PATCH RFC bpf-next 1/3] bpf: add mapper macro for bpf_cmd enum

[Andrii Nakryiko]

On Mon, Dec 11, 2023 at 6:40 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Dec 7, 2023 at 2:28 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > +#define __BPF_CMD_MAPPER(FN, ctx...)                                   \
> > +       FN(BPF_MAP_CREATE, 0)                                           \
> > +       FN(BPF_MAP_LOOKUP_ELEM, 1)                                      \
> > +       FN(BPF_MAP_UPDATE_ELEM, 2)                                      \
> > +       FN(BPF_MAP_DELETE_ELEM, 3)                                      \
> > +       FN(BPF_MAP_GET_NEXT_KEY, 4)                                     \
>
> So macro conversion across 4 main enums in uapi/bpf.h
> is just to do:
> +static const struct constant_table cmd_kvs[] = {
> +       __BPF_CMD_MAPPER(__BPF_KV_FN)
> +       {}
> +};
>
> on the kernel side,
> right?

Right.

>
> While in libbpf we already hard code name to value in arrays:
> prog_type_name[], map_type_name[]
>
> which probably will remain as-is, since libbpf needs to be
> built independently from the kernel.
> (unless we will say that tools/uapi/bpf.h is part of libbpf,
> which probably not a good way).

No, we can easily use this on the libbpf side as well. Libbpf syncs
the latest UAPI headers ([0]) and uses them during build time.

  [0] https://github.com/libbpf/libbpf/tree/master/include/uapi/linux

>
> There are more pros than cons in this enum uglification,
> but cons are definitely staring in the face.
>
> Have you considered other options?
> Like using vmlinix BTF for parsing bpffs delegation?
> [14083] ENUM 'bpf_cmd' encoding=UNSIGNED size=4 vlen=39
>         'BPF_MAP_CREATE' val=0
>         'BPF_MAP_LOOKUP_ELEM' val=1
>         'BPF_MAP_UPDATE_ELEM' val=2
>         'BPF_MAP_DELETE_ELEM' val=3
>         'BPF_MAP_GET_NEXT_KEY' val=4
>         'BPF_PROG_LOAD' val=5
>
> Names and values are available.
> btf_find_by_name_kind(vmlinux_btf, "bpd_cmd", BTF_KIND_ENUM);
> is fast enough.
>
> I suspect you'll argue that you don't want to tie in
> bpffs delegation parsing with BTF ;)
>

Yep, that's the reason I didn't go for it in the initial version, of
course. But it's fine.

> While I can preemptively answer that in the case vmlinux BTF
> is not available it's fine not to parse names and rely on hex.

It's fine, I can do optional BTF-based parsing, if that's what you prefer.

Re: [PATCH RFC bpf-next 1/3] bpf: add mapper macro for bpf_cmd enum

[Alexei Starovoitov]

On Mon, Dec 11, 2023 at 8:01 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
>
> > While I can preemptively answer that in the case vmlinux BTF
> > is not available it's fine not to parse names and rely on hex.
>
> It's fine, I can do optional BTF-based parsing, if that's what you prefer.

I prefer to keep uapi/bpf.h as-is and use BTF.
But I'd like to hear what Daniel's and Martin's preferences are.

Re: [PATCH RFC bpf-next 1/3] bpf: add mapper macro for bpf_cmd enum

[Martin KaFai Lau]

On 12/11/23 8:06 PM, Alexei Starovoitov wrote:
> On Mon, Dec 11, 2023 at 8:01 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>>
>>> While I can preemptively answer that in the case vmlinux BTF
>>> is not available it's fine not to parse names and rely on hex.
>>
>> It's fine, I can do optional BTF-based parsing, if that's what you prefer.
> 
> I prefer to keep uapi/bpf.h as-is and use BTF.
> But I'd like to hear what Daniel's and Martin's preferences are.

I think user will find it useful to have a more readable uapi header file. It 
would be nice to keep the current uapi/bpf.h form if there is another solution.

Re: [PATCH RFC bpf-next 1/3] bpf: add mapper macro for bpf_cmd enum

[Andrii Nakryiko]

On Tue, Dec 12, 2023 at 5:37 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 12/11/23 8:06 PM, Alexei Starovoitov wrote:
> > On Mon, Dec 11, 2023 at 8:01 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >>
> >>> While I can preemptively answer that in the case vmlinux BTF
> >>> is not available it's fine not to parse names and rely on hex.
> >>
> >> It's fine, I can do optional BTF-based parsing, if that's what you prefer.
> >
> > I prefer to keep uapi/bpf.h as-is and use BTF.
> > But I'd like to hear what Daniel's and Martin's preferences are.
>
> I think user will find it useful to have a more readable uapi header file. It

I'd say having numeric values make it more readable, but that's a
separate discussion. I purposefully kept full BPF_-prefixed names
intact for readability, as opposed to what we do for enum bpf_func_id.

> would be nice to keep the current uapi/bpf.h form if there is another solution.

Ok, I'll use BTF, no problem.