* [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported
@ 2020-04-14 4:56 Andrii Nakryiko
2020-04-14 7:03 ` Song Liu
2020-04-14 17:56 ` Andrey Ignatov
0 siblings, 2 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2020-04-14 4:56 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, rdna
Cc: andrii.nakryiko, kernel-team, 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>
---
v1->v2:
- fixed prog_type/expected_attach_type combo (Andrey);
- added comment explaining what we are doing in probe_exp_attach_type (Andrey).
tools/lib/bpf/libbpf.c | 127 ++++++++++++------
.../selftests/bpf/prog_tests/section_names.c | 42 +++---
2 files changed, 110 insertions(+), 59 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ff9174282a8c..c7393182e2ae 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,37 @@ 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));
+ /* use any valid combination of program type and (optional)
+ * non-zero expected attach type (i.e., not a BPF_CGROUP_INET_INGRESS)
+ * to see if kernel supports expected_attach_type field for
+ * BPF_PROG_LOAD command
+ */
+ attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
+ attr.expected_attach_type = BPF_CGROUP_INET_SOCK_CREATE;
+ 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 +3375,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 +4912,12 @@ 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;
if (prog->caps->name)
load_attr.name = prog->name;
load_attr.insns = insns;
@@ -5062,6 +5118,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 +5175,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 +6274,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 +6314,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 +6325,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 +6758,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 +7587,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 +7605,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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported
2020-04-14 4:56 [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported Andrii Nakryiko
@ 2020-04-14 7:03 ` Song Liu
2020-04-14 17:43 ` Andrii Nakryiko
2020-04-14 17:56 ` Andrey Ignatov
1 sibling, 1 reply; 7+ messages in thread
From: Song Liu @ 2020-04-14 7:03 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, daniel, Andrey Ignatov,
andrii.nakryiko, Kernel Team
> On Apr 13, 2020, at 9:56 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>
> 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>
Acked-by: Song Liu <songliubraving@fb.com>
With one nit below.
> ---
> v1->v2:
> - fixed prog_type/expected_attach_type combo (Andrey);
> - added comment explaining what we are doing in probe_exp_attach_type (Andrey).
>
> tools/lib/bpf/libbpf.c | 127 ++++++++++++------
> .../selftests/bpf/prog_tests/section_names.c | 42 +++---
> 2 files changed, 110 insertions(+), 59 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ff9174282a8c..c7393182e2ae 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;
> };
[...]
> -#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, \
Two lines of ".sec = string".
> + .expected_attach_type = eatype, \
> + .is_exp_attach_type_optional = eatype_optional, \
> + .is_attachable = attachable, \
> + .is_attach_btf = attach_btf, \
> + }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported
2020-04-14 7:03 ` Song Liu
@ 2020-04-14 17:43 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2020-04-14 17:43 UTC (permalink / raw)
To: Song Liu
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, daniel,
Andrey Ignatov, Kernel Team
On Tue, Apr 14, 2020 at 12:04 AM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Apr 13, 2020, at 9:56 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > 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>
>
> Acked-by: Song Liu <songliubraving@fb.com>
>
> With one nit below.
>
> > ---
> > v1->v2:
> > - fixed prog_type/expected_attach_type combo (Andrey);
> > - added comment explaining what we are doing in probe_exp_attach_type (Andrey).
> >
> > tools/lib/bpf/libbpf.c | 127 ++++++++++++------
> > .../selftests/bpf/prog_tests/section_names.c | 42 +++---
> > 2 files changed, 110 insertions(+), 59 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index ff9174282a8c..c7393182e2ae 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;
> > };
>
> [...]
>
> > -#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, \
>
> Two lines of ".sec = string".
*facepalm*, will fix in next version, once bpf-next is open.
>
> > + .expected_attach_type = eatype, \
> > + .is_exp_attach_type_optional = eatype_optional, \
> > + .is_attachable = attachable, \
> > + .is_attach_btf = attach_btf, \
> > + }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported
2020-04-14 4:56 [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported Andrii Nakryiko
2020-04-14 7:03 ` Song Liu
@ 2020-04-14 17:56 ` Andrey Ignatov
2020-04-14 18:24 ` Andrii Nakryiko
1 sibling, 1 reply; 7+ messages in thread
From: Andrey Ignatov @ 2020-04-14 17:56 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team
Andrii Nakryiko <andriin@fb.com> [Mon, 2020-04-13 21:56 -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>
> ---
> v1->v2:
> - fixed prog_type/expected_attach_type combo (Andrey);
> - added comment explaining what we are doing in probe_exp_attach_type (Andrey).
Thanks for changes.
I built the patch (removing the double .sec Song mentioned since it
breaks compilation) and tested it: it fixes the problem with NET_XMIT_CN
on old kernels and works for me with cgroup skb on old kernels.
Thank you!
Acked-by: Andrey Ignatov <rdna@fb.com>
I guess we can deal with selftest separately in the original thread.
Also a question about bpf vs bpf-next: since this fixes real problem
with loading cgroup skb programs, should it go to bpf tree instead?
> tools/lib/bpf/libbpf.c | 127 ++++++++++++------
> .../selftests/bpf/prog_tests/section_names.c | 42 +++---
> 2 files changed, 110 insertions(+), 59 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ff9174282a8c..c7393182e2ae 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,37 @@ 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));
> + /* use any valid combination of program type and (optional)
> + * non-zero expected attach type (i.e., not a BPF_CGROUP_INET_INGRESS)
> + * to see if kernel supports expected_attach_type field for
> + * BPF_PROG_LOAD command
> + */
> + attr.prog_type = BPF_PROG_TYPE_CGROUP_SOCK;
> + attr.expected_attach_type = BPF_CGROUP_INET_SOCK_CREATE;
> + 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 +3375,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 +4912,12 @@ 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;
> if (prog->caps->name)
> load_attr.name = prog->name;
> load_attr.insns = insns;
> @@ -5062,6 +5118,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 +5175,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 +6274,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 +6314,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 +6325,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 +6758,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 +7587,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 +7605,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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported
2020-04-14 17:56 ` Andrey Ignatov
@ 2020-04-14 18:24 ` Andrii Nakryiko
2020-04-14 18:43 ` Andrey Ignatov
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-04-14 18:24 UTC (permalink / raw)
To: Andrey Ignatov
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Tue, Apr 14, 2020 at 10:56 AM Andrey Ignatov <rdna@fb.com> wrote:
>
> Andrii Nakryiko <andriin@fb.com> [Mon, 2020-04-13 21:56 -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>
> > ---
> > v1->v2:
> > - fixed prog_type/expected_attach_type combo (Andrey);
> > - added comment explaining what we are doing in probe_exp_attach_type (Andrey).
>
> Thanks for changes.
>
> I built the patch (removing the double .sec Song mentioned since it
> breaks compilation) and tested it: it fixes the problem with NET_XMIT_CN
Wait, what? How does it break compilation? I compiled and tested
before submitting and just cleaned and built again, no compilation
errors or even warnings. Can you share compilation error you got,
please?
> on old kernels and works for me with cgroup skb on old kernels.
>
> Thank you!
>
> Acked-by: Andrey Ignatov <rdna@fb.com>
Thanks!
>
> I guess we can deal with selftest separately in the original thread.
Sure, if this is going to be applied to bpf as a fix, I'd rather
follow-up with selftests separately.
>
> Also a question about bpf vs bpf-next: since this fixes real problem
> with loading cgroup skb programs, should it go to bpf tree instead?
It will be up to maintainers, it's not so clear whether it's a new
feature or a bug fix.. I don't mind either way.
>
>
> > tools/lib/bpf/libbpf.c | 127 ++++++++++++------
> > .../selftests/bpf/prog_tests/section_names.c | 42 +++---
> > 2 files changed, 110 insertions(+), 59 deletions(-)
> >
[...]
trimming :)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported
2020-04-14 18:24 ` Andrii Nakryiko
@ 2020-04-14 18:43 ` Andrey Ignatov
2020-04-14 19:38 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Andrey Ignatov @ 2020-04-14 18:43 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
Andrii Nakryiko <andrii.nakryiko@gmail.com> [Tue, 2020-04-14 11:25 -0700]:
> On Tue, Apr 14, 2020 at 10:56 AM Andrey Ignatov <rdna@fb.com> wrote:
> >
> > Andrii Nakryiko <andriin@fb.com> [Mon, 2020-04-13 21:56 -0700]:
...
> > > v1->v2:
> > > - fixed prog_type/expected_attach_type combo (Andrey);
> > > - added comment explaining what we are doing in probe_exp_attach_type (Andrey).
> >
> > Thanks for changes.
> >
> > I built the patch (removing the double .sec Song mentioned since it
> > breaks compilation) and tested it: it fixes the problem with NET_XMIT_CN
>
> Wait, what? How does it break compilation? I compiled and tested
> before submitting and just cleaned and built again, no compilation
> errors or even warnings. Can you share compilation error you got,
> please?
Sure:
11:37:28 1 rdna@dev082.prn2:~/bpf-next$>/home/rdna/bin/clang --version
clang version 9.0.20190721
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /home/rdna/bin
11:37:32 0 rdna@dev082.prn2:~/bpf-next$>env GCC=~/bin/gcc CLANG=~/bin/clang CC=~/bin/clang LLC=~/bin/llc LLVM_STRIP=~/bin/llvm-strip make V=1 -C tools/bpf/bpftool/
Auto-detecting system features:
... libbfd: [ on ]
... disassembler-four-args: [ OFF ]
... zlib: [ on ]
... clang-bpf-global-var: [ on ]
make: Entering directory `/home/rdna/bpf-next/tools/bpf/bpftool'
make -C /home/rdna/bpf-next/tools/lib/bpf/ OUTPUT= libbpf.a
make[1]: Entering directory `/home/rdna/bpf-next/tools/lib/bpf'
make -f /home/rdna/bpf-next/tools/build/Makefile.build dir=. obj=libbpf OUTPUT=staticobjs/
/home/rdna/bin/clang -Wp,-MD,staticobjs/.libbpf.o.d -Wp,-MT,staticobjs/libbpf.o -g -Wall -DHAVE_LIBELF_MMAP_SUPPORT -DCOMPAT_NEED_REALLOCARRAY -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes -Wswitch-default -Wswitch-enum -Wundef -Wwrite-strings -Wformat -fno-strict-aliasing -Wno-shadow -Werror -Wall -fPIC -I. -I/home/rdna/bpf-next/tools/include -I/home/rdna/bpf-next/tools/arch/x86/include/uapi -I/home/rdna/bpf-next/tools/include/uapi -fvisibility=hidden -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D"BUILD_STR(s)=#s" -c -o staticobjs/libbpf.o libbpf.c
libbpf.c:6329:15: error: initializer overrides prior initialization of this subobject [-Werror,-Winitializer-overrides]
BPF_PROG_SEC("socket", BPF_PROG_TYPE_SOCKET_FILTER),
^~~~~~~~
libbpf.c:6291:55: note: expanded from macro 'BPF_PROG_SEC'
#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
^~~~~~
libbpf.c:6283:10: note: expanded from macro 'BPF_PROG_SEC_IMPL'
.sec = string, \
^~~~~~
libbpf.c:6329:15: note: previous initialization is here
BPF_PROG_SEC("socket", BPF_PROG_TYPE_SOCKET_FILTER),
^~~~~~~~
libbpf.c:6291:55: note: expanded from macro 'BPF_PROG_SEC'
#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
^~~~~~
libbpf.c:6280:10: note: expanded from macro 'BPF_PROG_SEC_IMPL'
.sec = string, \
^~~~~~
libbpf.c:6330:15: error: initializer overrides prior initialization of this subobject [-Werror,-Winitializer-overrides]
BPF_PROG_SEC("sk_reuseport", BPF_PROG_TYPE_SK_REUSEPORT),
^~~~~~~~~~~~~~
libbpf.c:6291:55: note: expanded from macro 'BPF_PROG_SEC'
#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
^~~~~~
libbpf.c:6283:10: note: expanded from macro 'BPF_PROG_SEC_IMPL'
.sec = string, \
^~~~~~
libbpf.c:6330:15: note: previous initialization is here
BPF_PROG_SEC("sk_reuseport", BPF_PROG_TYPE_SK_REUSEPORT),
^~~~~~~~~~~~~~
libbpf.c:6291:55: note: expanded from macro 'BPF_PROG_SEC'
#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
^~~~~~
libbpf.c:6280:10: note: expanded from macro 'BPF_PROG_SEC_IMPL'
.sec = string, \
^~~~~~
... and so on ...
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
ld -r -o staticobjs/libbpf-in.o staticobjs/libbpf.o staticobjs/bpf.o staticobjs/nlattr.o staticobjs/btf.o staticobjs/libbpf_errno.o staticobjs/str_error.o staticobjs/netlink.o staticobjs/bpf_prog_linfo.o staticobjs/libbpf_probes.o staticobjs/xsk.o staticobjs/hashmap.o staticobjs/btf_dump.o
ld: cannot find staticobjs/libbpf.o: No such file or directory
make[2]: *** [staticobjs/libbpf-in.o] Error 1
make[1]: *** [staticobjs/libbpf-in.o] Error 2
make[1]: Leaving directory `/home/rdna/bpf-next/tools/lib/bpf'
make: *** [/home/rdna/bpf-next/tools/lib/bpf/libbpf.a] Error 2
make: Leaving directory `/home/rdna/bpf-next/tools/bpf/bpftool'
11:37:43 2 rdna@dev082.prn2:~/bpf-next$>
> > I guess we can deal with selftest separately in the original thread.
>
> Sure, if this is going to be applied to bpf as a fix, I'd rather
> follow-up with selftests separately.
Sounds good.
> > Also a question about bpf vs bpf-next: since this fixes real problem
> > with loading cgroup skb programs, should it go to bpf tree instead?
>
> It will be up to maintainers, it's not so clear whether it's a new
> feature or a bug fix.. I don't mind either way.
Sounds good. Thanks.
--
Andrey Ignatov
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported
2020-04-14 18:43 ` Andrey Ignatov
@ 2020-04-14 19:38 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2020-04-14 19:38 UTC (permalink / raw)
To: Andrey Ignatov
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Tue, Apr 14, 2020 at 11:44 AM Andrey Ignatov <rdna@fb.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> [Tue, 2020-04-14 11:25 -0700]:
> > On Tue, Apr 14, 2020 at 10:56 AM Andrey Ignatov <rdna@fb.com> wrote:
> > >
> > > Andrii Nakryiko <andriin@fb.com> [Mon, 2020-04-13 21:56 -0700]:
>
> ...
>
> > > > v1->v2:
> > > > - fixed prog_type/expected_attach_type combo (Andrey);
> > > > - added comment explaining what we are doing in probe_exp_attach_type (Andrey).
> > >
> > > Thanks for changes.
> > >
> > > I built the patch (removing the double .sec Song mentioned since it
> > > breaks compilation) and tested it: it fixes the problem with NET_XMIT_CN
> >
> > Wait, what? How does it break compilation? I compiled and tested
> > before submitting and just cleaned and built again, no compilation
> > errors or even warnings. Can you share compilation error you got,
> > please?
>
> Sure:
>
> 11:37:28 1 rdna@dev082.prn2:~/bpf-next$>/home/rdna/bin/clang --version
> clang version 9.0.20190721
> Target: x86_64-unknown-linux-gnu
> Thread model: posix
> InstalledDir: /home/rdna/bin
> 11:37:32 0 rdna@dev082.prn2:~/bpf-next$>env GCC=~/bin/gcc CLANG=~/bin/clang CC=~/bin/clang LLC=~/bin/llc LLVM_STRIP=~/bin/llvm-strip make V=1 -C tools/bpf/bpftool/
[...]
>
> fatal error: too many errors emitted, stopping now [-ferror-limit=]
> 20 errors generated.
> ld -r -o staticobjs/libbpf-in.o staticobjs/libbpf.o staticobjs/bpf.o staticobjs/nlattr.o staticobjs/btf.o staticobjs/libbpf_errno.o staticobjs/str_error.o staticobjs/netlink.o staticobjs/bpf_prog_linfo.o staticobjs/libbpf_probes.o staticobjs/xsk.o staticobjs/hashmap.o staticobjs/btf_dump.o
> ld: cannot find staticobjs/libbpf.o: No such file or directory
> make[2]: *** [staticobjs/libbpf-in.o] Error 1
> make[1]: *** [staticobjs/libbpf-in.o] Error 2
> make[1]: Leaving directory `/home/rdna/bpf-next/tools/lib/bpf'
> make: *** [/home/rdna/bpf-next/tools/lib/bpf/libbpf.a] Error 2
> make: Leaving directory `/home/rdna/bpf-next/tools/bpf/bpftool'
> 11:37:43 2 rdna@dev082.prn2:~/bpf-next$>
Weird, I can't repro it locally neither with GCC, nor with clang-9 or
latest clang...
>
> > > I guess we can deal with selftest separately in the original thread.
> >
> > Sure, if this is going to be applied to bpf as a fix, I'd rather
> > follow-up with selftests separately.
>
> Sounds good.
>
> > > Also a question about bpf vs bpf-next: since this fixes real problem
> > > with loading cgroup skb programs, should it go to bpf tree instead?
> >
> > It will be up to maintainers, it's not so clear whether it's a new
> > feature or a bug fix.. I don't mind either way.
>
> Sounds good. Thanks.
>
> --
> Andrey Ignatov
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-14 19:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-14 4:56 [PATCH v2 bpf-next] libbpf: always specify expected_attach_type on program load if supported Andrii Nakryiko
2020-04-14 7:03 ` Song Liu
2020-04-14 17:43 ` Andrii Nakryiko
2020-04-14 17:56 ` Andrey Ignatov
2020-04-14 18:24 ` Andrii Nakryiko
2020-04-14 18:43 ` Andrey Ignatov
2020-04-14 19:38 ` Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.