[PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[Andrii Nakryiko]

For some types of BPF programs that utilize expected_attach_type, libbpf won't
set load_attr.expected_attach_type, even if expected_attach_type is known from
section definition. This was done to preserve backwards compatibility with old
kernels that didn't recognize expected_attach_type attribute yet (which was
added in 5e43f899b03a ("bpf: Check attach type at prog load time"). But this
is problematic for some BPF programs that utilize never features that require
kernel to know specific expected_attach_type (e.g., extended set of return
codes for cgroup_skb/egress programs).

This patch makes libbpf specify expected_attach_type by default, but also
detect support for this field in kernel and not set it during program load.
This allows to have a good metadata for bpf_program
(e.g., bpf_program__get_extected_attach_type()), but still work with old
kernels (for cases where it can work at all).

Additionally, due to expected_attach_type being always set for recognized
program types, bpf_program__attach_cgroup doesn't have to do extra checks to
determine correct attach type, so remove that additional logic.

Also adjust section_names selftest to account for this change.

More detailed discussion can be found in [0].

  [0] https://lore.kernel.org/bpf/20200412003604.GA15986@rdna-mbp.dhcp.thefacebook.com/

Reported-by: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c                        | 123 +++++++++++-------
 .../selftests/bpf/prog_tests/section_names.c  |  42 +++---
 2 files changed, 106 insertions(+), 59 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ff9174282a8c..925f720deea0 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -178,6 +178,8 @@ struct bpf_capabilities {
 	__u32 array_mmap:1;
 	/* BTF_FUNC_GLOBAL is supported */
 	__u32 btf_func_global:1;
+	/* kernel support for expected_attach_type in BPF_PROG_LOAD */
+	__u32 exp_attach_type:1;
 };
 
 enum reloc_type {
@@ -194,6 +196,22 @@ struct reloc_desc {
 	int sym_off;
 };
 
+struct bpf_sec_def;
+
+typedef struct bpf_link *(*attach_fn_t)(const struct bpf_sec_def *sec,
+					struct bpf_program *prog);
+
+struct bpf_sec_def {
+	const char *sec;
+	size_t len;
+	enum bpf_prog_type prog_type;
+	enum bpf_attach_type expected_attach_type;
+	bool is_exp_attach_type_optional;
+	bool is_attachable;
+	bool is_attach_btf;
+	attach_fn_t attach_fn;
+};
+
 /*
  * bpf_prog should be a better name but it has been used in
  * linux/filter.h.
@@ -204,6 +222,7 @@ struct bpf_program {
 	char *name;
 	int prog_ifindex;
 	char *section_name;
+	const struct bpf_sec_def *sec_def;
 	/* section_name with / replaced by _; makes recursive pinning
 	 * in bpf_object__pin_programs easier
 	 */
@@ -3315,6 +3334,32 @@ static int bpf_object__probe_array_mmap(struct bpf_object *obj)
 	return 0;
 }
 
+static int
+bpf_object__probe_exp_attach_type(struct bpf_object *obj)
+{
+	struct bpf_load_program_attr attr;
+	struct bpf_insn insns[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+	int fd;
+
+	memset(&attr, 0, sizeof(attr));
+	attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
+	attr.expected_attach_type = BPF_CGROUP_INET_EGRESS;
+	attr.insns = insns;
+	attr.insns_cnt = ARRAY_SIZE(insns);
+	attr.license = "GPL";
+
+	fd = bpf_load_program_xattr(&attr, NULL, 0);
+	if (fd >= 0) {
+		obj->caps.exp_attach_type = 1;
+		close(fd);
+		return 1;
+	}
+	return 0;
+}
+
 static int
 bpf_object__probe_caps(struct bpf_object *obj)
 {
@@ -3325,6 +3370,7 @@ bpf_object__probe_caps(struct bpf_object *obj)
 		bpf_object__probe_btf_func_global,
 		bpf_object__probe_btf_datasec,
 		bpf_object__probe_array_mmap,
+		bpf_object__probe_exp_attach_type,
 	};
 	int i, ret;
 
@@ -4861,7 +4907,13 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 
 	memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
 	load_attr.prog_type = prog->type;
-	load_attr.expected_attach_type = prog->expected_attach_type;
+	/* old kernels might not support specifying expected_attach_type */
+	if (!prog->caps->exp_attach_type && prog->sec_def &&
+	    prog->sec_def->is_exp_attach_type_optional)
+		load_attr.expected_attach_type = 0;
+	else
+		load_attr.expected_attach_type = prog->expected_attach_type;
+	pr_warn("prog %s exp_Attach %d\n", prog->name, load_attr.expected_attach_type);
 	if (prog->caps->name)
 		load_attr.name = prog->name;
 	load_attr.insns = insns;
@@ -5062,6 +5114,8 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
 	return 0;
 }
 
+static const struct bpf_sec_def *find_sec_def(const char *sec_name);
+
 static struct bpf_object *
 __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 		   const struct bpf_object_open_opts *opts)
@@ -5117,24 +5171,17 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 	bpf_object__elf_finish(obj);
 
 	bpf_object__for_each_program(prog, obj) {
-		enum bpf_prog_type prog_type;
-		enum bpf_attach_type attach_type;
-
-		if (prog->type != BPF_PROG_TYPE_UNSPEC)
-			continue;
-
-		err = libbpf_prog_type_by_name(prog->section_name, &prog_type,
-					       &attach_type);
-		if (err == -ESRCH)
+		prog->sec_def = find_sec_def(prog->section_name);
+		if (!prog->sec_def)
 			/* couldn't guess, but user might manually specify */
 			continue;
-		if (err)
-			goto out;
 
-		bpf_program__set_type(prog, prog_type);
-		bpf_program__set_expected_attach_type(prog, attach_type);
-		if (prog_type == BPF_PROG_TYPE_TRACING ||
-		    prog_type == BPF_PROG_TYPE_EXT)
+		bpf_program__set_type(prog, prog->sec_def->prog_type);
+		bpf_program__set_expected_attach_type(prog,
+				prog->sec_def->expected_attach_type);
+
+		if (prog->sec_def->prog_type == BPF_PROG_TYPE_TRACING ||
+		    prog->sec_def->prog_type == BPF_PROG_TYPE_EXT)
 			prog->attach_prog_fd = OPTS_GET(opts, attach_prog_fd, 0);
 	}
 
@@ -6223,23 +6270,33 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 	prog->expected_attach_type = type;
 }
 
-#define BPF_PROG_SEC_IMPL(string, ptype, eatype, is_attachable, btf, atype) \
-	{ string, sizeof(string) - 1, ptype, eatype, is_attachable, btf, atype }
+#define BPF_PROG_SEC_IMPL(string, ptype, eatype, eatype_optional,	    \
+			  attachable, attach_btf)			    \
+	{								    \
+		.sec = string,						    \
+		.len = sizeof(string) - 1,				    \
+		.prog_type = ptype,					    \
+		.sec = string,						    \
+		.expected_attach_type = eatype,				    \
+		.is_exp_attach_type_optional = eatype_optional,		    \
+		.is_attachable = attachable,				    \
+		.is_attach_btf = attach_btf,				    \
+	}
 
 /* Programs that can NOT be attached. */
 #define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
 
 /* Programs that can be attached. */
 #define BPF_APROG_SEC(string, ptype, atype) \
-	BPF_PROG_SEC_IMPL(string, ptype, 0, 1, 0, atype)
+	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
 
 /* Programs that must specify expected attach type at load time. */
 #define BPF_EAPROG_SEC(string, ptype, eatype) \
-	BPF_PROG_SEC_IMPL(string, ptype, eatype, 1, 0, eatype)
+	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 1, 0)
 
 /* Programs that use BTF to identify attach point */
 #define BPF_PROG_BTF(string, ptype, eatype) \
-	BPF_PROG_SEC_IMPL(string, ptype, eatype, 0, 1, 0)
+	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
 
 /* Programs that can be attached but attach type can't be identified by section
  * name. Kept for backward compatibility.
@@ -6253,11 +6310,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
 	__VA_ARGS__							    \
 }
 
-struct bpf_sec_def;
-
-typedef struct bpf_link *(*attach_fn_t)(const struct bpf_sec_def *sec,
-					struct bpf_program *prog);
-
 static struct bpf_link *attach_kprobe(const struct bpf_sec_def *sec,
 				      struct bpf_program *prog);
 static struct bpf_link *attach_tp(const struct bpf_sec_def *sec,
@@ -6269,17 +6321,6 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
 static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
 				   struct bpf_program *prog);
 
-struct bpf_sec_def {
-	const char *sec;
-	size_t len;
-	enum bpf_prog_type prog_type;
-	enum bpf_attach_type expected_attach_type;
-	bool is_attachable;
-	bool is_attach_btf;
-	enum bpf_attach_type attach_type;
-	attach_fn_t attach_fn;
-};
-
 static const struct bpf_sec_def section_defs[] = {
 	BPF_PROG_SEC("socket",			BPF_PROG_TYPE_SOCKET_FILTER),
 	BPF_PROG_SEC("sk_reuseport",		BPF_PROG_TYPE_SK_REUSEPORT),
@@ -6713,7 +6754,7 @@ int libbpf_attach_type_by_name(const char *name,
 			continue;
 		if (!section_defs[i].is_attachable)
 			return -EINVAL;
-		*attach_type = section_defs[i].attach_type;
+		*attach_type = section_defs[i].expected_attach_type;
 		return 0;
 	}
 	pr_debug("failed to guess attach type based on ELF section name '%s'\n", name);
@@ -7542,7 +7583,6 @@ static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
 struct bpf_link *
 bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
 {
-	const struct bpf_sec_def *sec_def;
 	enum bpf_attach_type attach_type;
 	char errmsg[STRERR_BUFSIZE];
 	struct bpf_link *link;
@@ -7561,11 +7601,6 @@ bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
 	link->detach = &bpf_link__detach_fd;
 
 	attach_type = bpf_program__get_expected_attach_type(prog);
-	if (!attach_type) {
-		sec_def = find_sec_def(bpf_program__title(prog, false));
-		if (sec_def)
-			attach_type = sec_def->attach_type;
-	}
 	link_fd = bpf_link_create(prog_fd, cgroup_fd, attach_type, NULL);
 	if (link_fd < 0) {
 		link_fd = -errno;
diff --git a/tools/testing/selftests/bpf/prog_tests/section_names.c b/tools/testing/selftests/bpf/prog_tests/section_names.c
index 9d9351dc2ded..713167449c98 100644
--- a/tools/testing/selftests/bpf/prog_tests/section_names.c
+++ b/tools/testing/selftests/bpf/prog_tests/section_names.c
@@ -43,18 +43,18 @@ static struct sec_name_test tests[] = {
 	{"lwt_seg6local", {0, BPF_PROG_TYPE_LWT_SEG6LOCAL, 0}, {-EINVAL, 0} },
 	{
 		"cgroup_skb/ingress",
-		{0, BPF_PROG_TYPE_CGROUP_SKB, 0},
+		{0, BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_INGRESS},
 		{0, BPF_CGROUP_INET_INGRESS},
 	},
 	{
 		"cgroup_skb/egress",
-		{0, BPF_PROG_TYPE_CGROUP_SKB, 0},
+		{0, BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_EGRESS},
 		{0, BPF_CGROUP_INET_EGRESS},
 	},
 	{"cgroup/skb", {0, BPF_PROG_TYPE_CGROUP_SKB, 0}, {-EINVAL, 0} },
 	{
 		"cgroup/sock",
-		{0, BPF_PROG_TYPE_CGROUP_SOCK, 0},
+		{0, BPF_PROG_TYPE_CGROUP_SOCK, BPF_CGROUP_INET_SOCK_CREATE},
 		{0, BPF_CGROUP_INET_SOCK_CREATE},
 	},
 	{
@@ -69,26 +69,38 @@ static struct sec_name_test tests[] = {
 	},
 	{
 		"cgroup/dev",
-		{0, BPF_PROG_TYPE_CGROUP_DEVICE, 0},
+		{0, BPF_PROG_TYPE_CGROUP_DEVICE, BPF_CGROUP_DEVICE},
 		{0, BPF_CGROUP_DEVICE},
 	},
-	{"sockops", {0, BPF_PROG_TYPE_SOCK_OPS, 0}, {0, BPF_CGROUP_SOCK_OPS} },
+	{
+		"sockops",
+		{0, BPF_PROG_TYPE_SOCK_OPS, BPF_CGROUP_SOCK_OPS},
+		{0, BPF_CGROUP_SOCK_OPS},
+	},
 	{
 		"sk_skb/stream_parser",
-		{0, BPF_PROG_TYPE_SK_SKB, 0},
+		{0, BPF_PROG_TYPE_SK_SKB, BPF_SK_SKB_STREAM_PARSER},
 		{0, BPF_SK_SKB_STREAM_PARSER},
 	},
 	{
 		"sk_skb/stream_verdict",
-		{0, BPF_PROG_TYPE_SK_SKB, 0},
+		{0, BPF_PROG_TYPE_SK_SKB, BPF_SK_SKB_STREAM_VERDICT},
 		{0, BPF_SK_SKB_STREAM_VERDICT},
 	},
 	{"sk_skb", {0, BPF_PROG_TYPE_SK_SKB, 0}, {-EINVAL, 0} },
-	{"sk_msg", {0, BPF_PROG_TYPE_SK_MSG, 0}, {0, BPF_SK_MSG_VERDICT} },
-	{"lirc_mode2", {0, BPF_PROG_TYPE_LIRC_MODE2, 0}, {0, BPF_LIRC_MODE2} },
+	{
+		"sk_msg",
+		{0, BPF_PROG_TYPE_SK_MSG, BPF_SK_MSG_VERDICT},
+		{0, BPF_SK_MSG_VERDICT},
+	},
+	{
+		"lirc_mode2",
+		{0, BPF_PROG_TYPE_LIRC_MODE2, BPF_LIRC_MODE2},
+		{0, BPF_LIRC_MODE2},
+	},
 	{
 		"flow_dissector",
-		{0, BPF_PROG_TYPE_FLOW_DISSECTOR, 0},
+		{0, BPF_PROG_TYPE_FLOW_DISSECTOR, BPF_FLOW_DISSECTOR},
 		{0, BPF_FLOW_DISSECTOR},
 	},
 	{
@@ -158,17 +170,17 @@ static void test_prog_type_by_name(const struct sec_name_test *test)
 				      &expected_attach_type);
 
 	CHECK(rc != test->expected_load.rc, "check_code",
-	      "prog: unexpected rc=%d for %s", rc, test->sec_name);
+	      "prog: unexpected rc=%d for %s\n", rc, test->sec_name);
 
 	if (rc)
 		return;
 
 	CHECK(prog_type != test->expected_load.prog_type, "check_prog_type",
-	      "prog: unexpected prog_type=%d for %s",
+	      "prog: unexpected prog_type=%d for %s\n",
 	      prog_type, test->sec_name);
 
 	CHECK(expected_attach_type != test->expected_load.expected_attach_type,
-	      "check_attach_type", "prog: unexpected expected_attach_type=%d for %s",
+	      "check_attach_type", "prog: unexpected expected_attach_type=%d for %s\n",
 	      expected_attach_type, test->sec_name);
 }
 
@@ -180,13 +192,13 @@ static void test_attach_type_by_name(const struct sec_name_test *test)
 	rc = libbpf_attach_type_by_name(test->sec_name, &attach_type);
 
 	CHECK(rc != test->expected_attach.rc, "check_ret",
-	      "attach: unexpected rc=%d for %s", rc, test->sec_name);
+	      "attach: unexpected rc=%d for %s\n", rc, test->sec_name);
 
 	if (rc)
 		return;
 
 	CHECK(attach_type != test->expected_attach.attach_type,
-	      "check_attach_type", "attach: unexpected attach_type=%d for %s",
+	      "check_attach_type", "attach: unexpected attach_type=%d for %s\n",
 	      attach_type, test->sec_name);
 }
 
-- 
2.24.1

Re: [PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[Andrey Ignatov]

Andrii Nakryiko <andriin@fb.com> [Sat, 2020-04-11 22:58 -0700]:
> For some types of BPF programs that utilize expected_attach_type, libbpf won't
> set load_attr.expected_attach_type, even if expected_attach_type is known from
> section definition. This was done to preserve backwards compatibility with old
> kernels that didn't recognize expected_attach_type attribute yet (which was
> added in 5e43f899b03a ("bpf: Check attach type at prog load time"). But this
> is problematic for some BPF programs that utilize never features that require
> kernel to know specific expected_attach_type (e.g., extended set of return
> codes for cgroup_skb/egress programs).
> 
> This patch makes libbpf specify expected_attach_type by default, but also
> detect support for this field in kernel and not set it during program load.
> This allows to have a good metadata for bpf_program
> (e.g., bpf_program__get_extected_attach_type()), but still work with old
> kernels (for cases where it can work at all).
> 
> Additionally, due to expected_attach_type being always set for recognized
> program types, bpf_program__attach_cgroup doesn't have to do extra checks to
> determine correct attach type, so remove that additional logic.
> 
> Also adjust section_names selftest to account for this change.
> 
> More detailed discussion can be found in [0].
> 
>   [0] https://lore.kernel.org/bpf/20200412003604.GA15986@rdna-mbp.dhcp.thefacebook.com/
> 
> Reported-by: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/lib/bpf/libbpf.c                        | 123 +++++++++++-------
>  .../selftests/bpf/prog_tests/section_names.c  |  42 +++---
>  2 files changed, 106 insertions(+), 59 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ff9174282a8c..925f720deea0 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -178,6 +178,8 @@ struct bpf_capabilities {
>  	__u32 array_mmap:1;
>  	/* BTF_FUNC_GLOBAL is supported */
>  	__u32 btf_func_global:1;
> +	/* kernel support for expected_attach_type in BPF_PROG_LOAD */
> +	__u32 exp_attach_type:1;
>  };
>  
>  enum reloc_type {
> @@ -194,6 +196,22 @@ struct reloc_desc {
>  	int sym_off;
>  };
>  
> +struct bpf_sec_def;
> +
> +typedef struct bpf_link *(*attach_fn_t)(const struct bpf_sec_def *sec,
> +					struct bpf_program *prog);
> +
> +struct bpf_sec_def {
> +	const char *sec;
> +	size_t len;
> +	enum bpf_prog_type prog_type;
> +	enum bpf_attach_type expected_attach_type;
> +	bool is_exp_attach_type_optional;
> +	bool is_attachable;
> +	bool is_attach_btf;
> +	attach_fn_t attach_fn;
> +};
> +
>  /*
>   * bpf_prog should be a better name but it has been used in
>   * linux/filter.h.
> @@ -204,6 +222,7 @@ struct bpf_program {
>  	char *name;
>  	int prog_ifindex;
>  	char *section_name;
> +	const struct bpf_sec_def *sec_def;
>  	/* section_name with / replaced by _; makes recursive pinning
>  	 * in bpf_object__pin_programs easier
>  	 */
> @@ -3315,6 +3334,32 @@ static int bpf_object__probe_array_mmap(struct bpf_object *obj)
>  	return 0;
>  }
>  
> +static int
> +bpf_object__probe_exp_attach_type(struct bpf_object *obj)
> +{
> +	struct bpf_load_program_attr attr;
> +	struct bpf_insn insns[] = {
> +		BPF_MOV64_IMM(BPF_REG_0, 0),
> +		BPF_EXIT_INSN(),
> +	};
> +	int fd;
> +
> +	memset(&attr, 0, sizeof(attr));
> +	attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
> +	attr.expected_attach_type = BPF_CGROUP_INET_EGRESS;

Could you clarify semantics of this function please?

According to the name it looks like it should check whether
expected_attach_type attribute is supported or not. But
BPF_CGROUP_INET_EGRESS doesn't align with this since
expected_attach_type itself was added long before it was supported for
BPF_CGROUP_INET_EGRESS.

For example 4fbac77d2d09 ("bpf: Hooks for sys_bind") added in Mar 2018 is
the first hook ever that used expected_attach_type.

aac3fc320d94 ("bpf: Post-hooks for sys_bind") added a bit later is the
first hook that made expected_attach_type optional (for
BPF_CGROUP_INET_SOCK_CREATE).

But 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") for
BPF_CGROUP_INET_EGRESS was merged more than a year after the previous
two. 

> +	attr.insns = insns;
> +	attr.insns_cnt = ARRAY_SIZE(insns);
> +	attr.license = "GPL";
> +
> +	fd = bpf_load_program_xattr(&attr, NULL, 0);
> +	if (fd >= 0) {
> +		obj->caps.exp_attach_type = 1;
> +		close(fd);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  static int
>  bpf_object__probe_caps(struct bpf_object *obj)
>  {
> @@ -3325,6 +3370,7 @@ bpf_object__probe_caps(struct bpf_object *obj)
>  		bpf_object__probe_btf_func_global,
>  		bpf_object__probe_btf_datasec,
>  		bpf_object__probe_array_mmap,
> +		bpf_object__probe_exp_attach_type,
>  	};
>  	int i, ret;
>  
> @@ -4861,7 +4907,13 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>  
>  	memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
>  	load_attr.prog_type = prog->type;
> -	load_attr.expected_attach_type = prog->expected_attach_type;
> +	/* old kernels might not support specifying expected_attach_type */
> +	if (!prog->caps->exp_attach_type && prog->sec_def &&
> +	    prog->sec_def->is_exp_attach_type_optional)
> +		load_attr.expected_attach_type = 0;
> +	else
> +		load_attr.expected_attach_type = prog->expected_attach_type;

I'm having a hard time checking whether it'll work for all cases or may
not work for some combination of prog/attach type and kernel version
since there are many subtleties.

For example BPF_PROG_TYPE_CGROUP_SOCK has both a hook where
expected_attach_type is optional (BPF_CGROUP_INET_SOCK_CREATE) and hooks
where it's required (BPF_CGROUP_INET{4,6}_POST_BIND), and there
bpf_prog_load_fixup_attach_type() function in always sets
expected_attach_type if it's not yet.

But I don't have context on all hooks that can be affected by this
change and could easily miss something.

Ideally it should be verified by tests. Current section_names.c test
only verifies what will be returned, but AFAIK there is no test that
checks whether provided combination of prog_type/expected_attach_type at
load time and attach_type at attach time would actually work both on
current and old kernels. Do you think it's possible to add such a
selftest? (current libbpf CI supports running on old kernels, doesn't
it?)


> +	pr_warn("prog %s exp_Attach %d\n", prog->name, load_attr.expected_attach_type);
>  	if (prog->caps->name)
>  		load_attr.name = prog->name;
>  	load_attr.insns = insns;
> @@ -5062,6 +5114,8 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
>  	return 0;
>  }
>  
> +static const struct bpf_sec_def *find_sec_def(const char *sec_name);
> +
>  static struct bpf_object *
>  __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>  		   const struct bpf_object_open_opts *opts)
> @@ -5117,24 +5171,17 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
>  	bpf_object__elf_finish(obj);
>  
>  	bpf_object__for_each_program(prog, obj) {
> -		enum bpf_prog_type prog_type;
> -		enum bpf_attach_type attach_type;
> -
> -		if (prog->type != BPF_PROG_TYPE_UNSPEC)
> -			continue;
> -
> -		err = libbpf_prog_type_by_name(prog->section_name, &prog_type,
> -					       &attach_type);
> -		if (err == -ESRCH)
> +		prog->sec_def = find_sec_def(prog->section_name);
> +		if (!prog->sec_def)
>  			/* couldn't guess, but user might manually specify */
>  			continue;
> -		if (err)
> -			goto out;
>  
> -		bpf_program__set_type(prog, prog_type);
> -		bpf_program__set_expected_attach_type(prog, attach_type);
> -		if (prog_type == BPF_PROG_TYPE_TRACING ||
> -		    prog_type == BPF_PROG_TYPE_EXT)
> +		bpf_program__set_type(prog, prog->sec_def->prog_type);
> +		bpf_program__set_expected_attach_type(prog,
> +				prog->sec_def->expected_attach_type);
> +
> +		if (prog->sec_def->prog_type == BPF_PROG_TYPE_TRACING ||
> +		    prog->sec_def->prog_type == BPF_PROG_TYPE_EXT)
>  			prog->attach_prog_fd = OPTS_GET(opts, attach_prog_fd, 0);
>  	}
>  
> @@ -6223,23 +6270,33 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  	prog->expected_attach_type = type;
>  }
>  
> -#define BPF_PROG_SEC_IMPL(string, ptype, eatype, is_attachable, btf, atype) \
> -	{ string, sizeof(string) - 1, ptype, eatype, is_attachable, btf, atype }
> +#define BPF_PROG_SEC_IMPL(string, ptype, eatype, eatype_optional,	    \
> +			  attachable, attach_btf)			    \
> +	{								    \
> +		.sec = string,						    \
> +		.len = sizeof(string) - 1,				    \
> +		.prog_type = ptype,					    \
> +		.sec = string,						    \
> +		.expected_attach_type = eatype,				    \
> +		.is_exp_attach_type_optional = eatype_optional,		    \
> +		.is_attachable = attachable,				    \
> +		.is_attach_btf = attach_btf,				    \
> +	}
>  
>  /* Programs that can NOT be attached. */
>  #define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
>  
>  /* Programs that can be attached. */
>  #define BPF_APROG_SEC(string, ptype, atype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, 0, 1, 0, atype)
> +	BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
>  
>  /* Programs that must specify expected attach type at load time. */
>  #define BPF_EAPROG_SEC(string, ptype, eatype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, eatype, 1, 0, eatype)
> +	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 1, 0)
>  
>  /* Programs that use BTF to identify attach point */
>  #define BPF_PROG_BTF(string, ptype, eatype) \
> -	BPF_PROG_SEC_IMPL(string, ptype, eatype, 0, 1, 0)
> +	BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
>  
>  /* Programs that can be attached but attach type can't be identified by section
>   * name. Kept for backward compatibility.
> @@ -6253,11 +6310,6 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
>  	__VA_ARGS__							    \
>  }
>  
> -struct bpf_sec_def;
> -
> -typedef struct bpf_link *(*attach_fn_t)(const struct bpf_sec_def *sec,
> -					struct bpf_program *prog);
> -
>  static struct bpf_link *attach_kprobe(const struct bpf_sec_def *sec,
>  				      struct bpf_program *prog);
>  static struct bpf_link *attach_tp(const struct bpf_sec_def *sec,
> @@ -6269,17 +6321,6 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
>  static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
>  				   struct bpf_program *prog);
>  
> -struct bpf_sec_def {
> -	const char *sec;
> -	size_t len;
> -	enum bpf_prog_type prog_type;
> -	enum bpf_attach_type expected_attach_type;
> -	bool is_attachable;
> -	bool is_attach_btf;
> -	enum bpf_attach_type attach_type;
> -	attach_fn_t attach_fn;
> -};
> -
>  static const struct bpf_sec_def section_defs[] = {
>  	BPF_PROG_SEC("socket",			BPF_PROG_TYPE_SOCKET_FILTER),
>  	BPF_PROG_SEC("sk_reuseport",		BPF_PROG_TYPE_SK_REUSEPORT),
> @@ -6713,7 +6754,7 @@ int libbpf_attach_type_by_name(const char *name,
>  			continue;
>  		if (!section_defs[i].is_attachable)
>  			return -EINVAL;
> -		*attach_type = section_defs[i].attach_type;
> +		*attach_type = section_defs[i].expected_attach_type;
>  		return 0;
>  	}
>  	pr_debug("failed to guess attach type based on ELF section name '%s'\n", name);
> @@ -7542,7 +7583,6 @@ static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
>  struct bpf_link *
>  bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
>  {
> -	const struct bpf_sec_def *sec_def;
>  	enum bpf_attach_type attach_type;
>  	char errmsg[STRERR_BUFSIZE];
>  	struct bpf_link *link;
> @@ -7561,11 +7601,6 @@ bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
>  	link->detach = &bpf_link__detach_fd;
>  
>  	attach_type = bpf_program__get_expected_attach_type(prog);
> -	if (!attach_type) {
> -		sec_def = find_sec_def(bpf_program__title(prog, false));
> -		if (sec_def)
> -			attach_type = sec_def->attach_type;
> -	}
>  	link_fd = bpf_link_create(prog_fd, cgroup_fd, attach_type, NULL);
>  	if (link_fd < 0) {
>  		link_fd = -errno;
> diff --git a/tools/testing/selftests/bpf/prog_tests/section_names.c b/tools/testing/selftests/bpf/prog_tests/section_names.c
> index 9d9351dc2ded..713167449c98 100644
> --- a/tools/testing/selftests/bpf/prog_tests/section_names.c
> +++ b/tools/testing/selftests/bpf/prog_tests/section_names.c
> @@ -43,18 +43,18 @@ static struct sec_name_test tests[] = {
>  	{"lwt_seg6local", {0, BPF_PROG_TYPE_LWT_SEG6LOCAL, 0}, {-EINVAL, 0} },
>  	{
>  		"cgroup_skb/ingress",
> -		{0, BPF_PROG_TYPE_CGROUP_SKB, 0},
> +		{0, BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_INGRESS},
>  		{0, BPF_CGROUP_INET_INGRESS},
>  	},
>  	{
>  		"cgroup_skb/egress",
> -		{0, BPF_PROG_TYPE_CGROUP_SKB, 0},
> +		{0, BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_EGRESS},
>  		{0, BPF_CGROUP_INET_EGRESS},
>  	},
>  	{"cgroup/skb", {0, BPF_PROG_TYPE_CGROUP_SKB, 0}, {-EINVAL, 0} },
>  	{
>  		"cgroup/sock",
> -		{0, BPF_PROG_TYPE_CGROUP_SOCK, 0},
> +		{0, BPF_PROG_TYPE_CGROUP_SOCK, BPF_CGROUP_INET_SOCK_CREATE},
>  		{0, BPF_CGROUP_INET_SOCK_CREATE},
>  	},
>  	{
> @@ -69,26 +69,38 @@ static struct sec_name_test tests[] = {
>  	},
>  	{
>  		"cgroup/dev",
> -		{0, BPF_PROG_TYPE_CGROUP_DEVICE, 0},
> +		{0, BPF_PROG_TYPE_CGROUP_DEVICE, BPF_CGROUP_DEVICE},
>  		{0, BPF_CGROUP_DEVICE},
>  	},
> -	{"sockops", {0, BPF_PROG_TYPE_SOCK_OPS, 0}, {0, BPF_CGROUP_SOCK_OPS} },
> +	{
> +		"sockops",
> +		{0, BPF_PROG_TYPE_SOCK_OPS, BPF_CGROUP_SOCK_OPS},
> +		{0, BPF_CGROUP_SOCK_OPS},
> +	},
>  	{
>  		"sk_skb/stream_parser",
> -		{0, BPF_PROG_TYPE_SK_SKB, 0},
> +		{0, BPF_PROG_TYPE_SK_SKB, BPF_SK_SKB_STREAM_PARSER},
>  		{0, BPF_SK_SKB_STREAM_PARSER},
>  	},
>  	{
>  		"sk_skb/stream_verdict",
> -		{0, BPF_PROG_TYPE_SK_SKB, 0},
> +		{0, BPF_PROG_TYPE_SK_SKB, BPF_SK_SKB_STREAM_VERDICT},
>  		{0, BPF_SK_SKB_STREAM_VERDICT},
>  	},
>  	{"sk_skb", {0, BPF_PROG_TYPE_SK_SKB, 0}, {-EINVAL, 0} },
> -	{"sk_msg", {0, BPF_PROG_TYPE_SK_MSG, 0}, {0, BPF_SK_MSG_VERDICT} },
> -	{"lirc_mode2", {0, BPF_PROG_TYPE_LIRC_MODE2, 0}, {0, BPF_LIRC_MODE2} },
> +	{
> +		"sk_msg",
> +		{0, BPF_PROG_TYPE_SK_MSG, BPF_SK_MSG_VERDICT},
> +		{0, BPF_SK_MSG_VERDICT},
> +	},
> +	{
> +		"lirc_mode2",
> +		{0, BPF_PROG_TYPE_LIRC_MODE2, BPF_LIRC_MODE2},
> +		{0, BPF_LIRC_MODE2},
> +	},
>  	{
>  		"flow_dissector",
> -		{0, BPF_PROG_TYPE_FLOW_DISSECTOR, 0},
> +		{0, BPF_PROG_TYPE_FLOW_DISSECTOR, BPF_FLOW_DISSECTOR},
>  		{0, BPF_FLOW_DISSECTOR},
>  	},
>  	{
> @@ -158,17 +170,17 @@ static void test_prog_type_by_name(const struct sec_name_test *test)
>  				      &expected_attach_type);
>  
>  	CHECK(rc != test->expected_load.rc, "check_code",
> -	      "prog: unexpected rc=%d for %s", rc, test->sec_name);
> +	      "prog: unexpected rc=%d for %s\n", rc, test->sec_name);
>  
>  	if (rc)
>  		return;
>  
>  	CHECK(prog_type != test->expected_load.prog_type, "check_prog_type",
> -	      "prog: unexpected prog_type=%d for %s",
> +	      "prog: unexpected prog_type=%d for %s\n",
>  	      prog_type, test->sec_name);
>  
>  	CHECK(expected_attach_type != test->expected_load.expected_attach_type,
> -	      "check_attach_type", "prog: unexpected expected_attach_type=%d for %s",
> +	      "check_attach_type", "prog: unexpected expected_attach_type=%d for %s\n",
>  	      expected_attach_type, test->sec_name);
>  }
>  
> @@ -180,13 +192,13 @@ static void test_attach_type_by_name(const struct sec_name_test *test)
>  	rc = libbpf_attach_type_by_name(test->sec_name, &attach_type);
>  
>  	CHECK(rc != test->expected_attach.rc, "check_ret",
> -	      "attach: unexpected rc=%d for %s", rc, test->sec_name);
> +	      "attach: unexpected rc=%d for %s\n", rc, test->sec_name);
>  
>  	if (rc)
>  		return;
>  
>  	CHECK(attach_type != test->expected_attach.attach_type,
> -	      "check_attach_type", "attach: unexpected attach_type=%d for %s",
> +	      "check_attach_type", "attach: unexpected attach_type=%d for %s\n",
>  	      attach_type, test->sec_name);
>  }
>  
> -- 
> 2.24.1
> 

-- 
Andrey Ignatov

Re: [PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[Andrii Nakryiko]

On Mon, Apr 13, 2020 at 1:21 PM Andrey Ignatov <rdna@fb.com> wrote:
>
> Andrii Nakryiko <andriin@fb.com> [Sat, 2020-04-11 22:58 -0700]:
> > For some types of BPF programs that utilize expected_attach_type, libbpf won't
> > set load_attr.expected_attach_type, even if expected_attach_type is known from
> > section definition. This was done to preserve backwards compatibility with old
> > kernels that didn't recognize expected_attach_type attribute yet (which was
> > added in 5e43f899b03a ("bpf: Check attach type at prog load time"). But this
> > is problematic for some BPF programs that utilize never features that require
> > kernel to know specific expected_attach_type (e.g., extended set of return
> > codes for cgroup_skb/egress programs).
> >
> > This patch makes libbpf specify expected_attach_type by default, but also
> > detect support for this field in kernel and not set it during program load.
> > This allows to have a good metadata for bpf_program
> > (e.g., bpf_program__get_extected_attach_type()), but still work with old
> > kernels (for cases where it can work at all).
> >
> > Additionally, due to expected_attach_type being always set for recognized
> > program types, bpf_program__attach_cgroup doesn't have to do extra checks to
> > determine correct attach type, so remove that additional logic.
> >
> > Also adjust section_names selftest to account for this change.
> >
> > More detailed discussion can be found in [0].
> >
> >   [0] https://lore.kernel.org/bpf/20200412003604.GA15986@rdna-mbp.dhcp.thefacebook.com/
> >
> > Reported-by: Andrey Ignatov <rdna@fb.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c                        | 123 +++++++++++-------
> >  .../selftests/bpf/prog_tests/section_names.c  |  42 +++---
> >  2 files changed, 106 insertions(+), 59 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index ff9174282a8c..925f720deea0 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -178,6 +178,8 @@ struct bpf_capabilities {
> >       __u32 array_mmap:1;
> >       /* BTF_FUNC_GLOBAL is supported */
> >       __u32 btf_func_global:1;
> > +     /* kernel support for expected_attach_type in BPF_PROG_LOAD */
> > +     __u32 exp_attach_type:1;
> >  };
> >
> >  enum reloc_type {
> > @@ -194,6 +196,22 @@ struct reloc_desc {
> >       int sym_off;
> >  };
> >
> > +struct bpf_sec_def;
> > +
> > +typedef struct bpf_link *(*attach_fn_t)(const struct bpf_sec_def *sec,
> > +                                     struct bpf_program *prog);
> > +
> > +struct bpf_sec_def {
> > +     const char *sec;
> > +     size_t len;
> > +     enum bpf_prog_type prog_type;
> > +     enum bpf_attach_type expected_attach_type;
> > +     bool is_exp_attach_type_optional;
> > +     bool is_attachable;
> > +     bool is_attach_btf;
> > +     attach_fn_t attach_fn;
> > +};
> > +
> >  /*
> >   * bpf_prog should be a better name but it has been used in
> >   * linux/filter.h.
> > @@ -204,6 +222,7 @@ struct bpf_program {
> >       char *name;
> >       int prog_ifindex;
> >       char *section_name;
> > +     const struct bpf_sec_def *sec_def;
> >       /* section_name with / replaced by _; makes recursive pinning
> >        * in bpf_object__pin_programs easier
> >        */
> > @@ -3315,6 +3334,32 @@ static int bpf_object__probe_array_mmap(struct bpf_object *obj)
> >       return 0;
> >  }
> >
> > +static int
> > +bpf_object__probe_exp_attach_type(struct bpf_object *obj)
> > +{
> > +     struct bpf_load_program_attr attr;
> > +     struct bpf_insn insns[] = {
> > +             BPF_MOV64_IMM(BPF_REG_0, 0),
> > +             BPF_EXIT_INSN(),
> > +     };
> > +     int fd;
> > +
> > +     memset(&attr, 0, sizeof(attr));
> > +     attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
> > +     attr.expected_attach_type = BPF_CGROUP_INET_EGRESS;
>
> Could you clarify semantics of this function please?
>
> According to the name it looks like it should check whether
> expected_attach_type attribute is supported or not. But
> BPF_CGROUP_INET_EGRESS doesn't align with this since
> expected_attach_type itself was added long before it was supported for
> BPF_CGROUP_INET_EGRESS.
>
> For example 4fbac77d2d09 ("bpf: Hooks for sys_bind") added in Mar 2018 is
> the first hook ever that used expected_attach_type.
>
> aac3fc320d94 ("bpf: Post-hooks for sys_bind") added a bit later is the
> first hook that made expected_attach_type optional (for
> BPF_CGROUP_INET_SOCK_CREATE).
>
> But 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") for
> BPF_CGROUP_INET_EGRESS was merged more than a year after the previous
> two.

I'm checking if kernel is rejecting non-zero expected_attach_type
field in bpf_attr for BPF_PROG_LOAD command.

Before 5e43f899b03a ("bpf: Check attach type at prog load time"),
kernel would reject non-zero expected_attach_type because
expected_attach_type didn't exist in bpf_attr. So if that's the case,
we shouldn't specify expected_attach_type.

After that commit, BPF_CGROUP_INET_EGRESS for
BPF_PROG_TYPE_CGROUP_SOCK would be supported, even if it is optional,
so using that combination should work.

Did I miss something?

>
> > +     attr.insns = insns;
> > +     attr.insns_cnt = ARRAY_SIZE(insns);
> > +     attr.license = "GPL";
> > +
> > +     fd = bpf_load_program_xattr(&attr, NULL, 0);
> > +     if (fd >= 0) {
> > +             obj->caps.exp_attach_type = 1;
> > +             close(fd);
> > +             return 1;
> > +     }
> > +     return 0;
> > +}
> > +
> >  static int
> >  bpf_object__probe_caps(struct bpf_object *obj)
> >  {
> > @@ -3325,6 +3370,7 @@ bpf_object__probe_caps(struct bpf_object *obj)
> >               bpf_object__probe_btf_func_global,
> >               bpf_object__probe_btf_datasec,
> >               bpf_object__probe_array_mmap,
> > +             bpf_object__probe_exp_attach_type,
> >       };
> >       int i, ret;
> >
> > @@ -4861,7 +4907,13 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >
> >       memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
> >       load_attr.prog_type = prog->type;
> > -     load_attr.expected_attach_type = prog->expected_attach_type;
> > +     /* old kernels might not support specifying expected_attach_type */
> > +     if (!prog->caps->exp_attach_type && prog->sec_def &&
> > +         prog->sec_def->is_exp_attach_type_optional)
> > +             load_attr.expected_attach_type = 0;
> > +     else
> > +             load_attr.expected_attach_type = prog->expected_attach_type;
>
> I'm having a hard time checking whether it'll work for all cases or may
> not work for some combination of prog/attach type and kernel version
> since there are many subtleties.
>
> For example BPF_PROG_TYPE_CGROUP_SOCK has both a hook where
> expected_attach_type is optional (BPF_CGROUP_INET_SOCK_CREATE) and hooks
> where it's required (BPF_CGROUP_INET{4,6}_POST_BIND), and there
> bpf_prog_load_fixup_attach_type() function in always sets
> expected_attach_type if it's not yet.

Right, so we use the fact that they are allowed, even if optional.
Libbpf should provide correct expected_attach_type, according to
section definitions and kernel should be happy (unless user specified
wrong section name, of course, but we can't help that).

>
> But I don't have context on all hooks that can be affected by this
> change and could easily miss something.
>
> Ideally it should be verified by tests. Current section_names.c test
> only verifies what will be returned, but AFAIK there is no test that
> checks whether provided combination of prog_type/expected_attach_type at
> load time and attach_type at attach time would actually work both on
> current and old kernels. Do you think it's possible to add such a
> selftest? (current libbpf CI supports running on old kernels, doesn't
> it?)

So all the existing selftests are essentially verifying this, if run
on old kernel. I don't think libbpf currently runs tests on such old
kernels, though. But there is no extra selftest that we need to add,
because every single existing one will execute this piece of libbpf
logic.

>
>
> > +     pr_warn("prog %s exp_Attach %d\n", prog->name, load_attr.expected_attach_type);
> >       if (prog->caps->name)
> >               load_attr.name = prog->name;
> >       load_attr.insns = insns;
> > @@ -5062,6 +5114,8 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
> >       return 0;
> >  }
> >

trimming irrelevant parts is good ;)

[...]

Re: [PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[Andrey Ignatov]

Andrii Nakryiko <andrii.nakryiko@gmail.com> [Mon, 2020-04-13 15:00 -0700]:
> On Mon, Apr 13, 2020 at 1:21 PM Andrey Ignatov <rdna@fb.com> wrote:
> >
> > Andrii Nakryiko <andriin@fb.com> [Sat, 2020-04-11 22:58 -0700]:
> > > For some types of BPF programs that utilize expected_attach_type, libbpf won't
> > > set load_attr.expected_attach_type, even if expected_attach_type is known from
> > > section definition. This was done to preserve backwards compatibility with old
> > > kernels that didn't recognize expected_attach_type attribute yet (which was
> > > added in 5e43f899b03a ("bpf: Check attach type at prog load time"). But this
> > > is problematic for some BPF programs that utilize never features that require
> > > kernel to know specific expected_attach_type (e.g., extended set of return
> > > codes for cgroup_skb/egress programs).
> > >
> > > This patch makes libbpf specify expected_attach_type by default, but also
> > > detect support for this field in kernel and not set it during program load.
> > > This allows to have a good metadata for bpf_program
> > > (e.g., bpf_program__get_extected_attach_type()), but still work with old
> > > kernels (for cases where it can work at all).
> > >
> > > Additionally, due to expected_attach_type being always set for recognized
> > > program types, bpf_program__attach_cgroup doesn't have to do extra checks to
> > > determine correct attach type, so remove that additional logic.
> > >
> > > Also adjust section_names selftest to account for this change.
> > >
> > > More detailed discussion can be found in [0].
> > >
> > >   [0] https://lore.kernel.org/bpf/20200412003604.GA15986@rdna-mbp.dhcp.thefacebook.com/
> > >
> > > Reported-by: Andrey Ignatov <rdna@fb.com>
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > >  tools/lib/bpf/libbpf.c                        | 123 +++++++++++-------
> > >  .../selftests/bpf/prog_tests/section_names.c  |  42 +++---
> > >  2 files changed, 106 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index ff9174282a8c..925f720deea0 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -178,6 +178,8 @@ struct bpf_capabilities {
> > >       __u32 array_mmap:1;
> > >       /* BTF_FUNC_GLOBAL is supported */
> > >       __u32 btf_func_global:1;
> > > +     /* kernel support for expected_attach_type in BPF_PROG_LOAD */
> > > +     __u32 exp_attach_type:1;
> > >  };
> > >
> > >  enum reloc_type {
> > > @@ -194,6 +196,22 @@ struct reloc_desc {
> > >       int sym_off;
> > >  };
> > >
> > > +struct bpf_sec_def;
> > > +
> > > +typedef struct bpf_link *(*attach_fn_t)(const struct bpf_sec_def *sec,
> > > +                                     struct bpf_program *prog);
> > > +
> > > +struct bpf_sec_def {
> > > +     const char *sec;
> > > +     size_t len;
> > > +     enum bpf_prog_type prog_type;
> > > +     enum bpf_attach_type expected_attach_type;
> > > +     bool is_exp_attach_type_optional;
> > > +     bool is_attachable;
> > > +     bool is_attach_btf;
> > > +     attach_fn_t attach_fn;
> > > +};
> > > +
> > >  /*
> > >   * bpf_prog should be a better name but it has been used in
> > >   * linux/filter.h.
> > > @@ -204,6 +222,7 @@ struct bpf_program {
> > >       char *name;
> > >       int prog_ifindex;
> > >       char *section_name;
> > > +     const struct bpf_sec_def *sec_def;
> > >       /* section_name with / replaced by _; makes recursive pinning
> > >        * in bpf_object__pin_programs easier
> > >        */
> > > @@ -3315,6 +3334,32 @@ static int bpf_object__probe_array_mmap(struct bpf_object *obj)
> > >       return 0;
> > >  }
> > >
> > > +static int
> > > +bpf_object__probe_exp_attach_type(struct bpf_object *obj)
> > > +{
> > > +     struct bpf_load_program_attr attr;
> > > +     struct bpf_insn insns[] = {
> > > +             BPF_MOV64_IMM(BPF_REG_0, 0),
> > > +             BPF_EXIT_INSN(),
> > > +     };
> > > +     int fd;
> > > +
> > > +     memset(&attr, 0, sizeof(attr));
> > > +     attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
> > > +     attr.expected_attach_type = BPF_CGROUP_INET_EGRESS;
> >
> > Could you clarify semantics of this function please?
> >
> > According to the name it looks like it should check whether
> > expected_attach_type attribute is supported or not. But
> > BPF_CGROUP_INET_EGRESS doesn't align with this since
> > expected_attach_type itself was added long before it was supported for
> > BPF_CGROUP_INET_EGRESS.
> >
> > For example 4fbac77d2d09 ("bpf: Hooks for sys_bind") added in Mar 2018 is
> > the first hook ever that used expected_attach_type.
> >
> > aac3fc320d94 ("bpf: Post-hooks for sys_bind") added a bit later is the
> > first hook that made expected_attach_type optional (for
> > BPF_CGROUP_INET_SOCK_CREATE).
> >
> > But 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") for
> > BPF_CGROUP_INET_EGRESS was merged more than a year after the previous
> > two.
> 
> I'm checking if kernel is rejecting non-zero expected_attach_type
> field in bpf_attr for BPF_PROG_LOAD command.
> 
> Before 5e43f899b03a ("bpf: Check attach type at prog load time"),
> kernel would reject non-zero expected_attach_type because
> expected_attach_type didn't exist in bpf_attr. So if that's the case,
> we shouldn't specify expected_attach_type.
> 
> After that commit, BPF_CGROUP_INET_EGRESS for
> BPF_PROG_TYPE_CGROUP_SOCK would be supported, even if it is optional,
> so using that combination should work.
> 
> Did I miss something?

So you're saying that there is nothing special about
BPF_CGROUP_INET_EGRESS, you just need _any_ attach type and it just
happened so that you used this one.

That sounds fine, but could you clarify it with a comment please,
otherwise it looks confusing: "why BPF_CGROUP_INET_EGRESS and not some
other attach type, for example any of those which got optional
expected_attach_type earlier, what's so special about
BPF_CGROUP_INET_EGRESS".

Also, I just realized that this combination: BPF_PROG_TYPE_CGROUP_SOCK and
BPF_CGROUP_INET_EGRESS is incorrect: BPF_CGROUP_INET_EGRESS attach type
corresponds to BPF_PROG_TYPE_CGROUP_SKB prog type, not to
BPF_PROG_TYPE_CGROUP_SOCK. This call will always fail with EINVAL on new
kernels, because of this code in bpf_prog_load_check_attach:

	switch (prog_type) {
	case BPF_PROG_TYPE_CGROUP_SOCK:
		switch (expected_attach_type) {
		case BPF_CGROUP_INET_SOCK_CREATE:
		case BPF_CGROUP_INET4_POST_BIND:
		case BPF_CGROUP_INET6_POST_BIND:
			return 0;
		default:
			return -EINVAL;
		}

That should be fixed.

And since this has to be changed anyway I'd go with BPF_PROG_TYPE_CGROUP_SOCK
and BPF_CGROUP_INET_SOCK_CREATE combination since this is the very first
combination in kernel that relied on optional expected_attach_type -- that
would make more sense IMO. And clarifying comment as mentioned above :)

> > > +     attr.insns = insns;
> > > +     attr.insns_cnt = ARRAY_SIZE(insns);
> > > +     attr.license = "GPL";
> > > +
> > > +     fd = bpf_load_program_xattr(&attr, NULL, 0);
> > > +     if (fd >= 0) {
> > > +             obj->caps.exp_attach_type = 1;
> > > +             close(fd);
> > > +             return 1;
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > >  static int
> > >  bpf_object__probe_caps(struct bpf_object *obj)
> > >  {
> > > @@ -3325,6 +3370,7 @@ bpf_object__probe_caps(struct bpf_object *obj)
> > >               bpf_object__probe_btf_func_global,
> > >               bpf_object__probe_btf_datasec,
> > >               bpf_object__probe_array_mmap,
> > > +             bpf_object__probe_exp_attach_type,
> > >       };
> > >       int i, ret;
> > >
> > > @@ -4861,7 +4907,13 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > >
> > >       memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
> > >       load_attr.prog_type = prog->type;
> > > -     load_attr.expected_attach_type = prog->expected_attach_type;
> > > +     /* old kernels might not support specifying expected_attach_type */
> > > +     if (!prog->caps->exp_attach_type && prog->sec_def &&
> > > +         prog->sec_def->is_exp_attach_type_optional)
> > > +             load_attr.expected_attach_type = 0;
> > > +     else
> > > +             load_attr.expected_attach_type = prog->expected_attach_type;
> >
> > I'm having a hard time checking whether it'll work for all cases or may
> > not work for some combination of prog/attach type and kernel version
> > since there are many subtleties.
> >
> > For example BPF_PROG_TYPE_CGROUP_SOCK has both a hook where
> > expected_attach_type is optional (BPF_CGROUP_INET_SOCK_CREATE) and hooks
> > where it's required (BPF_CGROUP_INET{4,6}_POST_BIND), and there
> > bpf_prog_load_fixup_attach_type() function in always sets
> > expected_attach_type if it's not yet.
> 
> Right, so we use the fact that they are allowed, even if optional.
> Libbpf should provide correct expected_attach_type, according to
> section definitions and kernel should be happy (unless user specified
> wrong section name, of course, but we can't help that).
> 
> >
> > But I don't have context on all hooks that can be affected by this
> > change and could easily miss something.
> >
> > Ideally it should be verified by tests. Current section_names.c test
> > only verifies what will be returned, but AFAIK there is no test that
> > checks whether provided combination of prog_type/expected_attach_type at
> > load time and attach_type at attach time would actually work both on
> > current and old kernels. Do you think it's possible to add such a
> > selftest? (current libbpf CI supports running on old kernels, doesn't
> > it?)
> 
> So all the existing selftests are essentially verifying this, if run
> on old kernel. I don't think libbpf currently runs tests on such old
> kernels, though. But there is no extra selftest that we need to add,
> because every single existing one will execute this piece of libbpf
> logic.

Apparently existing tests didn't catch the very obvious bug with
BPF_PROG_TYPE_CGROUP_SOCK / BPF_CGROUP_INET_EGRESS invalid combination.

I think it'd be useful to start with at least basic test focused on
expected_attach_type. Then later extend it to new attach types when they're
being added and, ideally, to existing ones.


> > > +     pr_warn("prog %s exp_Attach %d\n", prog->name, load_attr.expected_attach_type);
> > >       if (prog->caps->name)
> > >               load_attr.name = prog->name;
> > >       load_attr.insns = insns;
> > > @@ -5062,6 +5114,8 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
> > >       return 0;
> > >  }
> > >
> 
> trimming irrelevant parts is good ;)

ack

-- 
Andrey Ignatov

Re: [PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[Andrii Nakryiko]

On Mon, Apr 13, 2020 at 3:44 PM Andrey Ignatov <rdna@fb.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> [Mon, 2020-04-13 15:00 -0700]:
> > On Mon, Apr 13, 2020 at 1:21 PM Andrey Ignatov <rdna@fb.com> wrote:
> > >
> > > Andrii Nakryiko <andriin@fb.com> [Sat, 2020-04-11 22:58 -0700]:
> > > > For some types of BPF programs that utilize expected_attach_type, libbpf won't
> > > > set load_attr.expected_attach_type, even if expected_attach_type is known from
> > > > section definition. This was done to preserve backwards compatibility with old
> > > > kernels that didn't recognize expected_attach_type attribute yet (which was
> > > > added in 5e43f899b03a ("bpf: Check attach type at prog load time"). But this
> > > > is problematic for some BPF programs that utilize never features that require
> > > > kernel to know specific expected_attach_type (e.g., extended set of return
> > > > codes for cgroup_skb/egress programs).
> > > >
> > > > This patch makes libbpf specify expected_attach_type by default, but also
> > > > detect support for this field in kernel and not set it during program load.
> > > > This allows to have a good metadata for bpf_program
> > > > (e.g., bpf_program__get_extected_attach_type()), but still work with old
> > > > kernels (for cases where it can work at all).
> > > >
> > > > Additionally, due to expected_attach_type being always set for recognized
> > > > program types, bpf_program__attach_cgroup doesn't have to do extra checks to
> > > > determine correct attach type, so remove that additional logic.
> > > >
> > > > Also adjust section_names selftest to account for this change.
> > > >
> > > > More detailed discussion can be found in [0].
> > > >
> > > >   [0] https://lore.kernel.org/bpf/20200412003604.GA15986@rdna-mbp.dhcp.thefacebook.com/
> > > >
> > > > Reported-by: Andrey Ignatov <rdna@fb.com>
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > ---
> > > >  tools/lib/bpf/libbpf.c                        | 123 +++++++++++-------
> > > >  .../selftests/bpf/prog_tests/section_names.c  |  42 +++---
> > > >  2 files changed, 106 insertions(+), 59 deletions(-)
> > > >
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index ff9174282a8c..925f720deea0 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -178,6 +178,8 @@ struct bpf_capabilities {
> > > >       __u32 array_mmap:1;
> > > >       /* BTF_FUNC_GLOBAL is supported */
> > > >       __u32 btf_func_global:1;
> > > > +     /* kernel support for expected_attach_type in BPF_PROG_LOAD */
> > > > +     __u32 exp_attach_type:1;
> > > >  };
> > > >
> > > >  enum reloc_type {
> > > > @@ -194,6 +196,22 @@ struct reloc_desc {
> > > >       int sym_off;
> > > >  };
> > > >
> > > > +struct bpf_sec_def;
> > > > +
> > > > +typedef struct bpf_link *(*attach_fn_t)(const struct bpf_sec_def *sec,
> > > > +                                     struct bpf_program *prog);
> > > > +
> > > > +struct bpf_sec_def {
> > > > +     const char *sec;
> > > > +     size_t len;
> > > > +     enum bpf_prog_type prog_type;
> > > > +     enum bpf_attach_type expected_attach_type;
> > > > +     bool is_exp_attach_type_optional;
> > > > +     bool is_attachable;
> > > > +     bool is_attach_btf;
> > > > +     attach_fn_t attach_fn;
> > > > +};
> > > > +
> > > >  /*
> > > >   * bpf_prog should be a better name but it has been used in
> > > >   * linux/filter.h.
> > > > @@ -204,6 +222,7 @@ struct bpf_program {
> > > >       char *name;
> > > >       int prog_ifindex;
> > > >       char *section_name;
> > > > +     const struct bpf_sec_def *sec_def;
> > > >       /* section_name with / replaced by _; makes recursive pinning
> > > >        * in bpf_object__pin_programs easier
> > > >        */
> > > > @@ -3315,6 +3334,32 @@ static int bpf_object__probe_array_mmap(struct bpf_object *obj)
> > > >       return 0;
> > > >  }
> > > >
> > > > +static int
> > > > +bpf_object__probe_exp_attach_type(struct bpf_object *obj)
> > > > +{
> > > > +     struct bpf_load_program_attr attr;
> > > > +     struct bpf_insn insns[] = {
> > > > +             BPF_MOV64_IMM(BPF_REG_0, 0),
> > > > +             BPF_EXIT_INSN(),
> > > > +     };
> > > > +     int fd;
> > > > +
> > > > +     memset(&attr, 0, sizeof(attr));
> > > > +     attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
> > > > +     attr.expected_attach_type = BPF_CGROUP_INET_EGRESS;
> > >
> > > Could you clarify semantics of this function please?
> > >
> > > According to the name it looks like it should check whether
> > > expected_attach_type attribute is supported or not. But
> > > BPF_CGROUP_INET_EGRESS doesn't align with this since
> > > expected_attach_type itself was added long before it was supported for
> > > BPF_CGROUP_INET_EGRESS.
> > >
> > > For example 4fbac77d2d09 ("bpf: Hooks for sys_bind") added in Mar 2018 is
> > > the first hook ever that used expected_attach_type.
> > >
> > > aac3fc320d94 ("bpf: Post-hooks for sys_bind") added a bit later is the
> > > first hook that made expected_attach_type optional (for
> > > BPF_CGROUP_INET_SOCK_CREATE).
> > >
> > > But 5cf1e9145630 ("bpf: cgroup inet skb programs can return 0 to 3") for
> > > BPF_CGROUP_INET_EGRESS was merged more than a year after the previous
> > > two.
> >
> > I'm checking if kernel is rejecting non-zero expected_attach_type
> > field in bpf_attr for BPF_PROG_LOAD command.
> >
> > Before 5e43f899b03a ("bpf: Check attach type at prog load time"),
> > kernel would reject non-zero expected_attach_type because
> > expected_attach_type didn't exist in bpf_attr. So if that's the case,
> > we shouldn't specify expected_attach_type.
> >
> > After that commit, BPF_CGROUP_INET_EGRESS for
> > BPF_PROG_TYPE_CGROUP_SOCK would be supported, even if it is optional,
> > so using that combination should work.
> >
> > Did I miss something?
>
> So you're saying that there is nothing special about
> BPF_CGROUP_INET_EGRESS, you just need _any_ attach type and it just
> happened so that you used this one.

Yes.

>
> That sounds fine, but could you clarify it with a comment please,
> otherwise it looks confusing: "why BPF_CGROUP_INET_EGRESS and not some
> other attach type, for example any of those which got optional
> expected_attach_type earlier, what's so special about
> BPF_CGROUP_INET_EGRESS".

Sure.

>
> Also, I just realized that this combination: BPF_PROG_TYPE_CGROUP_SOCK and
> BPF_CGROUP_INET_EGRESS is incorrect: BPF_CGROUP_INET_EGRESS attach type
> corresponds to BPF_PROG_TYPE_CGROUP_SKB prog type, not to
> BPF_PROG_TYPE_CGROUP_SOCK. This call will always fail with EINVAL on new
> kernels, because of this code in bpf_prog_load_check_attach:

Yep, not sure how I copy-pasted BPF_PROG_TYPE_CGROUP_SOCK instead of
BPF_PROG_TYPE_CGROUP_SKB... My vim-foo clearly failed me here :)

>
>         switch (prog_type) {
>         case BPF_PROG_TYPE_CGROUP_SOCK:
>                 switch (expected_attach_type) {
>                 case BPF_CGROUP_INET_SOCK_CREATE:
>                 case BPF_CGROUP_INET4_POST_BIND:
>                 case BPF_CGROUP_INET6_POST_BIND:
>                         return 0;
>                 default:
>                         return -EINVAL;
>                 }
>
> That should be fixed.
>
> And since this has to be changed anyway I'd go with BPF_PROG_TYPE_CGROUP_SOCK
> and BPF_CGROUP_INET_SOCK_CREATE combination since this is the very first
> combination in kernel that relied on optional expected_attach_type -- that
> would make more sense IMO. And clarifying comment as mentioned above :)

Sure, sounds good, I'll use this combination instead.

>
> > > > +     attr.insns = insns;
> > > > +     attr.insns_cnt = ARRAY_SIZE(insns);
> > > > +     attr.license = "GPL";
> > > > +
> > > > +     fd = bpf_load_program_xattr(&attr, NULL, 0);
> > > > +     if (fd >= 0) {
> > > > +             obj->caps.exp_attach_type = 1;
> > > > +             close(fd);
> > > > +             return 1;
> > > > +     }
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static int
> > > >  bpf_object__probe_caps(struct bpf_object *obj)
> > > >  {
> > > > @@ -3325,6 +3370,7 @@ bpf_object__probe_caps(struct bpf_object *obj)
> > > >               bpf_object__probe_btf_func_global,
> > > >               bpf_object__probe_btf_datasec,
> > > >               bpf_object__probe_array_mmap,
> > > > +             bpf_object__probe_exp_attach_type,
> > > >       };
> > > >       int i, ret;
> > > >
> > > > @@ -4861,7 +4907,13 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > > >
> > > >       memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
> > > >       load_attr.prog_type = prog->type;
> > > > -     load_attr.expected_attach_type = prog->expected_attach_type;
> > > > +     /* old kernels might not support specifying expected_attach_type */
> > > > +     if (!prog->caps->exp_attach_type && prog->sec_def &&
> > > > +         prog->sec_def->is_exp_attach_type_optional)
> > > > +             load_attr.expected_attach_type = 0;
> > > > +     else
> > > > +             load_attr.expected_attach_type = prog->expected_attach_type;
> > >
> > > I'm having a hard time checking whether it'll work for all cases or may
> > > not work for some combination of prog/attach type and kernel version
> > > since there are many subtleties.
> > >
> > > For example BPF_PROG_TYPE_CGROUP_SOCK has both a hook where
> > > expected_attach_type is optional (BPF_CGROUP_INET_SOCK_CREATE) and hooks
> > > where it's required (BPF_CGROUP_INET{4,6}_POST_BIND), and there
> > > bpf_prog_load_fixup_attach_type() function in always sets
> > > expected_attach_type if it's not yet.
> >
> > Right, so we use the fact that they are allowed, even if optional.
> > Libbpf should provide correct expected_attach_type, according to
> > section definitions and kernel should be happy (unless user specified
> > wrong section name, of course, but we can't help that).
> >
> > >
> > > But I don't have context on all hooks that can be affected by this
> > > change and could easily miss something.
> > >
> > > Ideally it should be verified by tests. Current section_names.c test
> > > only verifies what will be returned, but AFAIK there is no test that
> > > checks whether provided combination of prog_type/expected_attach_type at
> > > load time and attach_type at attach time would actually work both on
> > > current and old kernels. Do you think it's possible to add such a
> > > selftest? (current libbpf CI supports running on old kernels, doesn't
> > > it?)
> >
> > So all the existing selftests are essentially verifying this, if run
> > on old kernel. I don't think libbpf currently runs tests on such old
> > kernels, though. But there is no extra selftest that we need to add,
> > because every single existing one will execute this piece of libbpf
> > logic.
>
> Apparently existing tests didn't catch the very obvious bug with
> BPF_PROG_TYPE_CGROUP_SOCK / BPF_CGROUP_INET_EGRESS invalid combination.

Sigh.. yeah. I expected cgroup_link test to fail if that functionality
didn't work, but I missed that bpf_program__attach_cgroup() code will
use correct expected_attach_type, even if it's not provided to
BPF_PROG_LOAD.

>
> I think it'd be useful to start with at least basic test focused on
> expected_attach_type. Then later extend it to new attach types when they're
> being added and, ideally, to existing ones.

How this test should look like? I can make a test that will work only
on new kernel (e.g., by using cgroup program which needs
expected_attach_type), but it will fail on old kernels. There doesn't
seem to be a way to query expected_attach_type from kernel. Any hints
on how to make test that will pass on old and new kernels and will
validate expected_attach_type is passed properly?

>
>
> > > > +     pr_warn("prog %s exp_Attach %d\n", prog->name, load_attr.expected_attach_type);
> > > >       if (prog->caps->name)
> > > >               load_attr.name = prog->name;
> > > >       load_attr.insns = insns;
> > > > @@ -5062,6 +5114,8 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
> > > >       return 0;
> > > >  }
> > > >
> >
> > trimming irrelevant parts is good ;)
>
> ack
>
> --
> Andrey Ignatov

Re: [PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[Andrey Ignatov]

Andrii Nakryiko <andrii.nakryiko@gmail.com> [Mon, 2020-04-13 21:49 -0700]:
> On Mon, Apr 13, 2020 at 3:44 PM Andrey Ignatov <rdna@fb.com> wrote:
> >
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Mon, 2020-04-13 15:00 -0700]:
> > > On Mon, Apr 13, 2020 at 1:21 PM Andrey Ignatov <rdna@fb.com> wrote:
> > > >
> > > > Andrii Nakryiko <andriin@fb.com> [Sat, 2020-04-11 22:58 -0700]:

...

> > > >
> > > > But I don't have context on all hooks that can be affected by this
> > > > change and could easily miss something.
> > > >
> > > > Ideally it should be verified by tests. Current section_names.c test
> > > > only verifies what will be returned, but AFAIK there is no test that
> > > > checks whether provided combination of prog_type/expected_attach_type at
> > > > load time and attach_type at attach time would actually work both on
> > > > current and old kernels. Do you think it's possible to add such a
> > > > selftest? (current libbpf CI supports running on old kernels, doesn't
> > > > it?)
> > >
> > > So all the existing selftests are essentially verifying this, if run
> > > on old kernel. I don't think libbpf currently runs tests on such old
> > > kernels, though. But there is no extra selftest that we need to add,
> > > because every single existing one will execute this piece of libbpf
> > > logic.
> >
> > Apparently existing tests didn't catch the very obvious bug with
> > BPF_PROG_TYPE_CGROUP_SOCK / BPF_CGROUP_INET_EGRESS invalid combination.
> 
> Sigh.. yeah. I expected cgroup_link test to fail if that functionality
> didn't work, but I missed that bpf_program__attach_cgroup() code will
> use correct expected_attach_type, even if it's not provided to
> BPF_PROG_LOAD.
> 
> >
> > I think it'd be useful to start with at least basic test focused on
> > expected_attach_type. Then later extend it to new attach types when they're
> > being added and, ideally, to existing ones.
> 
> How this test should look like? I can make a test that will work only
> on new kernel (e.g., by using cgroup program which needs
> expected_attach_type), but it will fail on old kernels. There doesn't
> seem to be a way to query expected_attach_type from kernel. Any hints
> on how to make test that will pass on old and new kernels and will
> validate expected_attach_type is passed properly?

I think there should be two steps here:
1) make a test;
2) make the test work on old kernels;

The "1)" should be pretty straightforward: we can just have an object
with all possible section names and make sure it can be loaded. If
a program type can have different scenarios, IMO all scenarios should be
covered.

For example, part of the object for cgroup_skb can look like this:

	#include <linux/bpf.h>
	#include <bpf/bpf_helpers.h>
	
	char _license[] SEC("license") = "GPL";
	int _version SEC("version") = 0;
	
	SEC("cgroup_skb/ingress")
	int skb_ret1(struct __sk_buff *skb)
	{
	        return 1;
	}
	
	/* Support for ret > 1 has different expectations for expected_attach_type */
	SEC("cgroup_skb/ingress")
	int skb_ret1(struct __sk_buff *skb)
	{
		return 2;
	}
	
	SEC("cgroup_skb/egress")
	int skb_ret1(struct __sk_buff *skb)
	{
	        return 1;
	}
	
	/* Support for ret > 1 has different expectations for expected_attach_type */
	SEC("cgroup_skb/egress")
	int skb_ret1(struct __sk_buff *skb)
	{
		return 2;
	}
	
	/* Compat section name */
	SEC("cgroup/skb")
	int skb_ret1(struct __sk_buff *skb)
	{
	        return 1;
	}

	/* ... and then other sections .. */

Some time later attach step can be added according to what kind of
program it is (e.g. try to attach cgroup programs to a cgroup, etc).

IMO it'd be beneficial for libbpf to have such a simple/single test that
verifies the very basic thing: simple program for every supported
section name can be loaded.

And such a test would caught the initial problem with NET_XMIT_CN.

I checked whether all sections have at least one program in selftests
and found a bunch that don't:

	09:43:11 0 rdna@dev082.prn2:~/bpf-next$>sed -ne '/static const struct bpf_sec_def section_defs/,/^\};/p' tools/lib/bpf/libbpf.c | awk -F'"' 'NF == 3 {printf "SEC(\"%s\n", $2}' | sort > all_sec_names
	09:43:19 0 rdna@dev082.prn2:~/bpf-next$>head -n 5 all_sec_names
	SEC("action
	SEC("cgroup/bind4
	SEC("cgroup/bind6
	SEC("cgroup/connect4
	SEC("cgroup/connect6
	09:43:20 0 rdna@dev082.prn2:~/bpf-next$>diff -u all_sec_names <(git grep -ohf all_sec_names tools/testing/selftests/bpf/ | sort -u)
	--- all_sec_names       2020-04-14 09:43:19.552675629 -0700
	+++ /dev/fd/63  2020-04-14 09:43:30.967648496 -0700
	@@ -1,21 +1,13 @@
	-SEC("action
	-SEC("cgroup/bind4
	-SEC("cgroup/bind6
	 SEC("cgroup/connect4
	 SEC("cgroup/connect6
	 SEC("cgroup/dev
	 SEC("cgroup/getsockopt
	-SEC("cgroup/post_bind4
	-SEC("cgroup/post_bind6
	-SEC("cgroup/recvmsg4
	-SEC("cgroup/recvmsg6
	 SEC("cgroup/sendmsg4
	 SEC("cgroup/sendmsg6
	 SEC("cgroup/setsockopt
	 SEC("cgroup/skb
	 SEC("cgroup_skb/egress
	 SEC("cgroup_skb/ingress
	-SEC("cgroup/sock
	 SEC("cgroup/sysctl
	 SEC("classifier
	 SEC("fentry/
	@@ -27,10 +19,7 @@
	 SEC("kretprobe/
	 SEC("lirc_mode2
	 SEC("lsm/
	-SEC("lwt_in
	-SEC("lwt_out
	 SEC("lwt_seg6local
	-SEC("lwt_xmit
	 SEC("perf_event
	 SEC("raw_tp/
	 SEC("raw_tracepoint/

That simple test can provide coverage for such sections.

As for "2)" -- I agree, it's not that straightforward: there should be a
way to check for feature presence in the kernel and skip if feature is
not present.  AFAIK currently there is no such thing in bpf
selftests(?). IMO it's fine to postpone this step for later time.

What do you think?

-- 
Andrey Ignatov

Re: [PATCH bpf-next] libbpf: always specify expected_attach_type on program load if supported

[Andrii Nakryiko]

On Tue, Apr 14, 2020 at 10:36 AM Andrey Ignatov <rdna@fb.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> [Mon, 2020-04-13 21:49 -0700]:
> > On Mon, Apr 13, 2020 at 3:44 PM Andrey Ignatov <rdna@fb.com> wrote:
> > >
> > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Mon, 2020-04-13 15:00 -0700]:
> > > > On Mon, Apr 13, 2020 at 1:21 PM Andrey Ignatov <rdna@fb.com> wrote:
> > > > >
> > > > > Andrii Nakryiko <andriin@fb.com> [Sat, 2020-04-11 22:58 -0700]:
>
> ...
>
> > > > >
> > > > > But I don't have context on all hooks that can be affected by this
> > > > > change and could easily miss something.
> > > > >
> > > > > Ideally it should be verified by tests. Current section_names.c test
> > > > > only verifies what will be returned, but AFAIK there is no test that
> > > > > checks whether provided combination of prog_type/expected_attach_type at
> > > > > load time and attach_type at attach time would actually work both on
> > > > > current and old kernels. Do you think it's possible to add such a
> > > > > selftest? (current libbpf CI supports running on old kernels, doesn't
> > > > > it?)
> > > >
> > > > So all the existing selftests are essentially verifying this, if run
> > > > on old kernel. I don't think libbpf currently runs tests on such old
> > > > kernels, though. But there is no extra selftest that we need to add,
> > > > because every single existing one will execute this piece of libbpf
> > > > logic.
> > >
> > > Apparently existing tests didn't catch the very obvious bug with
> > > BPF_PROG_TYPE_CGROUP_SOCK / BPF_CGROUP_INET_EGRESS invalid combination.
> >
> > Sigh.. yeah. I expected cgroup_link test to fail if that functionality
> > didn't work, but I missed that bpf_program__attach_cgroup() code will
> > use correct expected_attach_type, even if it's not provided to
> > BPF_PROG_LOAD.
> >
> > >
> > > I think it'd be useful to start with at least basic test focused on
> > > expected_attach_type. Then later extend it to new attach types when they're
> > > being added and, ideally, to existing ones.
> >
> > How this test should look like? I can make a test that will work only
> > on new kernel (e.g., by using cgroup program which needs
> > expected_attach_type), but it will fail on old kernels. There doesn't
> > seem to be a way to query expected_attach_type from kernel. Any hints
> > on how to make test that will pass on old and new kernels and will
> > validate expected_attach_type is passed properly?
>
> I think there should be two steps here:
> 1) make a test;
> 2) make the test work on old kernels;
>
> The "1)" should be pretty straightforward: we can just have an object
> with all possible section names and make sure it can be loaded. If
> a program type can have different scenarios, IMO all scenarios should be
> covered.
>
> For example, part of the object for cgroup_skb can look like this:
>
>         #include <linux/bpf.h>
>         #include <bpf/bpf_helpers.h>
>
>         char _license[] SEC("license") = "GPL";
>         int _version SEC("version") = 0;
>
>         SEC("cgroup_skb/ingress")
>         int skb_ret1(struct __sk_buff *skb)
>         {
>                 return 1;
>         }
>
>         /* Support for ret > 1 has different expectations for expected_attach_type */
>         SEC("cgroup_skb/ingress")
>         int skb_ret1(struct __sk_buff *skb)
>         {
>                 return 2;
>         }
>
>         SEC("cgroup_skb/egress")
>         int skb_ret1(struct __sk_buff *skb)
>         {
>                 return 1;
>         }
>
>         /* Support for ret > 1 has different expectations for expected_attach_type */
>         SEC("cgroup_skb/egress")
>         int skb_ret1(struct __sk_buff *skb)
>         {
>                 return 2;
>         }
>
>         /* Compat section name */
>         SEC("cgroup/skb")
>         int skb_ret1(struct __sk_buff *skb)
>         {
>                 return 1;
>         }
>
>         /* ... and then other sections .. */
>
> Some time later attach step can be added according to what kind of
> program it is (e.g. try to attach cgroup programs to a cgroup, etc).
>
> IMO it'd be beneficial for libbpf to have such a simple/single test that
> verifies the very basic thing: simple program for every supported
> section name can be loaded.
>
> And such a test would caught the initial problem with NET_XMIT_CN.


I agree. Though in my defense, those tests should have been added when
corresponding kernel features were added ;-P
Especially the test with [2, 3] returns for cgroup_skb progs. But I'll
prepare a selftest with trivial programs as you outlined them above.
And just test loading them, not attaching.


>
> I checked whether all sections have at least one program in selftests
> and found a bunch that don't:
>
>         09:43:11 0 rdna@dev082.prn2:~/bpf-next$>sed -ne '/static const struct bpf_sec_def section_defs/,/^\};/p' tools/lib/bpf/libbpf.c | awk -F'"' 'NF == 3 {printf "SEC(\"%s\n", $2}' | sort > all_sec_names
>         09:43:19 0 rdna@dev082.prn2:~/bpf-next$>head -n 5 all_sec_names
>         SEC("action
>         SEC("cgroup/bind4
>         SEC("cgroup/bind6
>         SEC("cgroup/connect4
>         SEC("cgroup/connect6
>         09:43:20 0 rdna@dev082.prn2:~/bpf-next$>diff -u all_sec_names <(git grep -ohf all_sec_names tools/testing/selftests/bpf/ | sort -u)
>         --- all_sec_names       2020-04-14 09:43:19.552675629 -0700
>         +++ /dev/fd/63  2020-04-14 09:43:30.967648496 -0700
>         @@ -1,21 +1,13 @@
>         -SEC("action
>         -SEC("cgroup/bind4
>         -SEC("cgroup/bind6
>          SEC("cgroup/connect4
>          SEC("cgroup/connect6
>          SEC("cgroup/dev
>          SEC("cgroup/getsockopt
>         -SEC("cgroup/post_bind4
>         -SEC("cgroup/post_bind6
>         -SEC("cgroup/recvmsg4
>         -SEC("cgroup/recvmsg6
>          SEC("cgroup/sendmsg4
>          SEC("cgroup/sendmsg6
>          SEC("cgroup/setsockopt
>          SEC("cgroup/skb
>          SEC("cgroup_skb/egress
>          SEC("cgroup_skb/ingress
>         -SEC("cgroup/sock
>          SEC("cgroup/sysctl
>          SEC("classifier
>          SEC("fentry/
>         @@ -27,10 +19,7 @@
>          SEC("kretprobe/
>          SEC("lirc_mode2
>          SEC("lsm/
>         -SEC("lwt_in
>         -SEC("lwt_out
>          SEC("lwt_seg6local
>         -SEC("lwt_xmit
>          SEC("perf_event
>          SEC("raw_tp/
>          SEC("raw_tracepoint/
>
> That simple test can provide coverage for such sections.

Actually, if we put all this in a single test, we'll have to disable
it for all kernel versions but the very last, because there will
always be some BPF program type that's not supported on even slightly
older kernel. So maybe for now, I'll just go and add a dummy
cgroup_skb/{ingress, egress} programs to existing selftests for
cgroup_skb programs.

>
> As for "2)" -- I agree, it's not that straightforward: there should be a
> way to check for feature presence in the kernel and skip if feature is
> not present.  AFAIK currently there is no such thing in bpf
> selftests(?). IMO it's fine to postpone this step for later time.
>
> What do you think?

See above, I think each type of program (or at least a class of
programs) should have its dedicated selftest. That way we can
blacklist the ones that are not supposed to succeed on particular
kernel. We do feature detection and skipping of the test only for
features that require very particular setup (e.g., hardware), but not
in general. It's been discussed many times, so far the tendency is to
not do feature detection and test disablement by default.

>
> --
> Andrey Ignatov