libbpf: fix attach of prog with multiple sections

When BPF programs which consists of multiple executable sections via iproute2+libbpf (configured with LIBBPF_FORCE=on), we noticed that a wrong section can be attached to a device. E.g.:

    # tc qdisc replace dev lxc_health clsact
    # tc filter replace dev lxc_health ingress prio 1 \
        handle 1 bpf da obj bpf_lxc.o sec from-container
    # tc filter show dev lxc_health ingress filter protocol all
        pref 1 bpf chain 0 filter protocol all pref 1 bpf chain 0
        handle 0x1 bpf_lxc.o:[__send_drop_notify] <-- WRONG SECTION
        direct-action not_in_hw id 38 tag 7d891814eda6809e jited

After taking a closer look into load_bpf_object() in lib/bpf_libbpf.c, we noticed that the filter used in the program iterator does not check whether a program section name matches a requested section name (cfg->section). This can lead to a wrong prog FD being used to attach the program.

Fixes 6d61a2b55799 ("lib: add libbpf support")

Signed-off-by Martynas Pumputis <[email protected]>

---
 lib/bpf_libbpf.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

lib/bpf_libbpf.c +6 -3

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
 	}
 
 	bpf_object__for_each_program(p, obj) {
+		bool prog_to_attach = !prog && cfg->section &&
+			!strcmp(get_bpf_program__section_name(p), cfg->section);
+
 		/* Only load the programs that will either be subsequently
 		 * attached or inserted into a tail call map */
-		if (find_legacy_tail_calls(p, obj) < 0 && cfg->section &&
-		    strcmp(get_bpf_program__section_name(p), cfg->section)) {
+		if (find_legacy_tail_calls(p, obj) < 0 && !prog_to_attach) {
 			ret = bpf_program__set_autoload(p, false);
 			if (ret)
 				return -EINVAL;

@@ -279,7 +281,8 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
 
 		bpf_program__set_type(p, cfg->type);
 		bpf_program__set_ifindex(p, cfg->ifindex);
-		if (!prog)
+
+		if (prog_to_attach)
 			prog = p;
 	}
 

Signature · 1 line

2.32.0

When BPF programs which consists of multiple executable sections via iproute2+libbpf (configured with LIBBPF_FORCE=on), we noticed that a wrong section can be attached to a device. E.g.:

    # tc qdisc replace dev lxc_health clsact
    # tc filter replace dev lxc_health ingress prio 1 \
        handle 1 bpf da obj bpf_lxc.o sec from-container
    # tc filter show dev lxc_health ingress filter protocol all
        pref 1 bpf chain 0 filter protocol all pref 1 bpf chain 0
        handle 0x1 bpf_lxc.o:[__send_drop_notify] <-- WRONG SECTION
        direct-action not_in_hw id 38 tag 7d891814eda6809e jited

After taking a closer look into load_bpf_object() in lib/bpf_libbpf.c, we noticed that the filter used in the program iterator does not check whether a program section name matches a requested section name (cfg->section). This can lead to a wrong prog FD being used to attach the program.

Fixes 6d61a2b55799 ("lib: add libbpf support")

Signed-off-by Martynas Pumputis <[email protected]>

---
 lib/bpf_libbpf.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644

lib/bpf_libbpf.c +4 -4

+
                /* Only load the programs that will either be subsequently
                 * attached or inserted into a tail call map */
-               if (find_legacy_tail_calls(p, obj) < 0 && cfg->section &&
-                   strcmp(get_bpf_program__section_name(p), cfg->section)) {
+               if (find_legacy_tail_calls(p, obj) < 0 && !prog_to_attach) {
                        ret = bpf_program__set_autoload(p, false);
                        if (ret)
                                return -EINVAL;

@@ -279,7 +281,8 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
 
                bpf_program__set_type(p, cfg->type);
                bpf_program__set_ifindex(p, cfg->ifindex);
-               if (!prog)
+
+               if (prog_to_attach)
                        prog = p;
        }
 
--

2.32.0

Quoted 32 lines · Martynas Pumputis

When BPF programs which consists of multiple executable sections via iproute2+libbpf (configured with LIBBPF_FORCE=on), we noticed that a wrong section can be attached to a device. E.g.:

     # tc qdisc replace dev lxc_health clsact
     # tc filter replace dev lxc_health ingress prio 1 \
         handle 1 bpf da obj bpf_lxc.o sec from-container
     # tc filter show dev lxc_health ingress filter protocol all
         pref 1 bpf chain 0 filter protocol all pref 1 bpf chain 0
         handle 0x1 bpf_lxc.o:[__send_drop_notify] <-- WRONG SECTION
         direct-action not_in_hw id 38 tag 7d891814eda6809e jited

After taking a closer look into load_bpf_object() in lib/bpf_libbpf.c, we noticed that the filter used in the program iterator does not check whether a program section name matches a requested section name (cfg->section). This can lead to a wrong prog FD being used to attach the program.

Fixes 6d61a2b55799 ("lib: add libbpf support")

Signed-off-by Martynas Pumputis <[email protected]>

---
  lib/bpf_libbpf.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
         }
 

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg) · line 271

Quoted 3 lines

lib/bpf_libbpf.c +2 -0

         bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p), cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Quoted 23 lines

lib/bpf_libbpf.c +4 -4

+
                 /* Only load the programs that will either be subsequently
                  * attached or inserted into a tail call map */
-               if (find_legacy_tail_calls(p, obj) < 0 && cfg->section &&
-                   strcmp(get_bpf_program__section_name(p), cfg->section)) {
+               if (find_legacy_tail_calls(p, obj) < 0 && !prog_to_attach) {
                         ret = bpf_program__set_autoload(p, false);
                         if (ret)
                                 return -EINVAL;

@@ -279,7 +281,8 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
 
                 bpf_program__set_type(p, cfg->type);
                 bpf_program__set_ifindex(p, cfg->ifindex);
-               if (!prog)
+
+               if (prog_to_attach)
                         prog = p;
         }
 
--

2.32.0

Quoted 7 lines

Quoted 7 lines

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
          }
 

Quoted 12 lines

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg)

Quoted 4 lines

lib/bpf_libbpf.c +2 -0

          bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p),
 cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

It would be interesting to hear thoughts from iproute2 maintainers before fixing this.

Quoted 25 lines · Martynas Pumputis

Quoted 7 lines

Quoted 7 lines

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
          }
 

Quoted 12 lines

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg)

Quoted 4 lines

lib/bpf_libbpf.c +2 -0

          bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p),
 cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 2 lines

It would be interesting to hear thoughts from iproute2 maintainers before fixing this.

maintaining backwards compatibility is a core principle for iproute2. If we know of a libbpf change is going to cause a breakage then it is best to fix it before any iproute2 release is affected.

Quoted 26 lines · David Ahern

Quoted 25 lines · Martynas Pumputis

Quoted 7 lines

Quoted 7 lines

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
          }
 

Quoted 12 lines

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg)

Quoted 4 lines

lib/bpf_libbpf.c +2 -0

          bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p),
 cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 7 lines

Quoted 2 lines

It would be interesting to hear thoughts from iproute2 maintainers before fixing this.

maintaining backwards compatibility is a core principle for iproute2. If we know of a libbpf change is going to cause a breakage then it is best to fix it before any iproute2 release is affected.

Just to avoid any confusion (if there is any), the required change we are discussing doesn't have anything to do with my fix.

To set the context, the motivation for unifying section names is documented and discussed in "Stricter and more uniform BPF program section name (SEC()) handling" of [1].

Andrii: is bpftool able to load programs with multiple sections which are named the same today?

[1]:
https://docs.google.com/document/d/1UyjTZuPFWiPFyKk1tV5an11_iaRuec6U-ZESZ54nNTY/edit#

Quoted 7 lines

Quoted 7 lines

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
         }
 

Quoted 11 lines

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg) · line 271

Quoted 3 lines

lib/bpf_libbpf.c +2 -0

         bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p), cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 19 lines · Martynas Pumputis

Quoted 18 lines

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg) · line 271

Quoted 10 lines

lib/bpf_libbpf.c +2 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
         }
 
         bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p), cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Quoted 3 lines

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Thanks
Hangbin

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 3 lines

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

Quoted 5 lines · Andrii Nakryiko

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are

I just propose an implementation as you suggested.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Quoted 3 lines

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

If you and others think it's OK to only support one program each section. I do no object.

Thanks
Hangbin

Quoted 21 lines · Hangbin Liu

Quoted 5 lines · Andrii Nakryiko

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are

I just propose an implementation as you suggested.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Quoted 7 lines

Quoted 3 lines

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

If you and others think it's OK to only support one program each section. I do no object.

I'm not sure we are on the same page. I'll try to summarize what I understood and you guys can decide for yourself what you want to do.

So I like your idea of introducing "prog" arg that will expect BPF program name (i.e., C function name). In that case the name is always unique. For existing "sec" arg, for backwards compatibility, I'd keep it working, but when "sec" is used I'd check that the match is unique (i.e., there is only one BPF program within the specified section). If not and there are more than one matching BPF programs, that's a hard error, because otherwise you essentially randomly pick one BPF program out of a few.

Quoted 25 lines · Hangbin Liu

Quoted 22 lines · Martynas Pumputis

Quoted 7 lines

Quoted 7 lines

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
         }
 

Quoted 11 lines

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg) · line 271

Quoted 3 lines

lib/bpf_libbpf.c +2 -0

         bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p), cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 3 lines

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

Quoted 26 lines · Andrii Nakryiko

Quoted 25 lines · Hangbin Liu

Quoted 22 lines · Martynas Pumputis

Quoted 7 lines

Quoted 7 lines

lib/bpf_libbpf.c +0 -0

index d05737a4..f76b90d2 100644
@@ -267,10 +267,12 @@ static int load_bpf_object(struct bpf_cfg_in *cfg)
         }
 

Quoted 11 lines

lib/bpf_libbpf.c static int load_bpf_object(struct bpf_cfg_in *cfg) · line 271

Quoted 3 lines

lib/bpf_libbpf.c +2 -0

         bpf_object__for_each_program(p, obj) {
+               bool prog_to_attach = !prog && cfg->section &&
+                       !strcmp(get_bpf_program__section_name(p), cfg->section);

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 8 lines

Quoted 3 lines

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

Let' see if I understand this correctly: libbpf 1.0 is not going to allow SEC("xdp_foo") or SEC("xdp_bar") kind of section names - which is the hint for libbpf to know program type. Instead only SEC("xdp") is allowed.

Further, a single object file is not going to be allowed to have multiple SEC("xdp") instances for each program name.

Correct? If so, it seems like this is limiting each object file to a single XDP program or a single object file can have 1 XDP program and 1 tc program.

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 8 lines

Quoted 3 lines

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

Quoted 4 lines

Let' see if I understand this correctly: libbpf 1.0 is not going to allow SEC("xdp_foo") or SEC("xdp_bar") kind of section names - which is the hint for libbpf to know program type. Instead only SEC("xdp") is allowed.

Right.

Quoted 1 line

Quoted 2 lines

Further, a single object file is not going to be allowed to have multiple SEC("xdp") instances for each program name.

On the contrary. Libbpf already allows (and will keep allowing) multiple BPF programs with SEC("xdp") in a single object file. Which is why section_name is not a unique program identifier.

Quoted 32 lines · Andrii Nakryiko

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 8 lines

Quoted 3 lines

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

Quoted 4 lines

Let' see if I understand this correctly: libbpf 1.0 is not going to allow SEC("xdp_foo") or SEC("xdp_bar") kind of section names - which is the hint for libbpf to know program type. Instead only SEC("xdp") is allowed.

Right.

Quoted 3 lines

Further, a single object file is not going to be allowed to have multiple SEC("xdp") instances for each program name.

Quoted 4 lines

On the contrary. Libbpf already allows (and will keep allowing) multiple BPF programs with SEC("xdp") in a single object file. Which is why section_name is not a unique program identifier.

Does that require BTF? My attempts at loading an object file with 2 SEC("xdp") programs failed. This is using bpftool from top of tree and loadall.

Quoted 34 lines · David Ahern

Quoted 32 lines · Andrii Nakryiko

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 8 lines

Quoted 3 lines

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

Quoted 4 lines

Let' see if I understand this correctly: libbpf 1.0 is not going to allow SEC("xdp_foo") or SEC("xdp_bar") kind of section names - which is the hint for libbpf to know program type. Instead only SEC("xdp") is allowed.

Right.

Quoted 3 lines

Further, a single object file is not going to be allowed to have multiple SEC("xdp") instances for each program name.

Quoted 8 lines

Quoted 4 lines

On the contrary. Libbpf already allows (and will keep allowing) multiple BPF programs with SEC("xdp") in a single object file. Which is why section_name is not a unique program identifier.

Does that require BTF? My attempts at loading an object file with 2 SEC("xdp") programs failed. This is using bpftool from top of tree and loadall.

You mean kernel BTF? Not if XDP programs themselves were built requiring CO-RE. So if those programs use #include "vmlinux.h", or there is BPF_CORE_READ() use somewhere in the code, or explicit __attribute__((preserve_access_index)) is used on some of the used structs, then yes, vmlinux BTF will be needed. But otherwise no. Do you have verbose error logs? I think with bpftool you can get them with -d argument.

Quoted 44 lines · Andrii Nakryiko

Quoted 34 lines · David Ahern

Quoted 32 lines · Andrii Nakryiko

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

This is still problematic, because one section can have multiple BPF programs. I.e., it's possible two define two or more XDP BPF programs all with SEC("xdp") and libbpf works just fine with that. I suggest moving users to specify the program name (i.e., C function name representing the BPF program). All the xdp_mycustom_suffix namings are a hack and will be rejected by libbpf 1.0, so it would be great to get a head start on fixing this early on.

Thanks for bringing this up. Currently, there is no way to specify a function name with "tc exec bpf" (only a section name via the "sec" arg). So probably, we should just add another arg to specify the function name.

Quoted 8 lines

Quoted 3 lines

How about add a "prog" arg to load specified program name and mark "sec" as not recommended? To keep backwards compatibility we just load the first program in the section.

Why not error out if there is more than one program with the same section name? if there is just one (and thus section name is still unique) -- then proceed. It seems much less confusing, IMO.

Quoted 4 lines

Let' see if I understand this correctly: libbpf 1.0 is not going to allow SEC("xdp_foo") or SEC("xdp_bar") kind of section names - which is the hint for libbpf to know program type. Instead only SEC("xdp") is allowed.

Right.

Quoted 3 lines

Further, a single object file is not going to be allowed to have multiple SEC("xdp") instances for each program name.

Quoted 8 lines

Quoted 4 lines

On the contrary. Libbpf already allows (and will keep allowing) multiple BPF programs with SEC("xdp") in a single object file. Which is why section_name is not a unique program identifier.

Does that require BTF? My attempts at loading an object file with 2 SEC("xdp") programs failed. This is using bpftool from top of tree and loadall.

Quoted 8 lines

You mean kernel BTF? Not if XDP programs themselves were built requiring CO-RE. So if those programs use #include "vmlinux.h", or there is BPF_CORE_READ() use somewhere in the code, or explicit __attribute__((preserve_access_index)) is used on some of the used structs, then yes, vmlinux BTF will be needed. But otherwise no. Do you have verbose error logs? I think with bpftool you can get them with -d argument.

xdp_l3fwd is built using an old school compile line - no CO-RE or BTF, just a basic compile line extracted from samples/bpf 2-3 years ago. Works fine for what I need and take this nothing more than an example to verify your comment

"Libbpf already allows (and will keep allowing) multiple BPF programs with SEC("xdp") in a single object file."

The bpftool command line to load the programs is:

$ bpftool -ddd prog loadall xdp_l3fwd.o /sys/fs/bpf

It fails because libbpf is trying to put 2 programs at the same path:

libbpf: pinned program '/sys/fs/bpf/xdp'
libbpf: failed to pin program: File exists
libbpf: unpinned program '/sys/fs/bpf/xdp'
Error: failed to pin all programs

The code that works is this:

SEC("xdp_l3fwd")
int xdp_l3fwd_prog(struct xdp_md *ctx)

{
        return xdp_l3fwd_flags(ctx, 0);
}

SEC("xdp_l3fwd_direct")
int xdp_l3fwd_direct_prog(struct xdp_md *ctx)

{
        return xdp_l3fwd_flags(ctx, BPF_FIB_LOOKUP_DIRECT);
}

The code that fails is this:

SEC("xdp")
int xdp_l3fwd_prog(struct xdp_md *ctx)

{
        return xdp_l3fwd_flags(ctx, 0);
}

SEC("xdp")
int xdp_l3fwd_direct_prog(struct xdp_md *ctx)

{
        return xdp_l3fwd_flags(ctx, BPF_FIB_LOOKUP_DIRECT);
}

which is what you said should work -- 2 programs with the same section name.

From a very quick check of bpftool vs libbpf, the former is calling bpf_object__pin_programs from the latter and passing the base path (/sys/fs/bpf in this example) and then bpf_object__pin_programs adds the pin_name for the prog - which must be the same for both programs since the second one fails.