| Message ID | 20210705124307.201303-1-m@lambda.lt |
|---|---|
| State | New |
| Headers | show |
| Series | [iproute2] libbpf: fix attach of prog with multiple sections | expand |
On Mon, Jul 05, 2021 at 02:43:07PM +0200, Martynas Pumputis wrote: > 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 <m@lambda.lt> > --- > lib/bpf_libbpf.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c > index d05737a4..f76b90d2 100644 > --- a/lib/bpf_libbpf.c > +++ b/lib/bpf_libbpf.c > @@ -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; > } > > -- > 2.32.0 > Thanks for the fix. Acked-by: Hangbin Liu <haliu@redhat.com>
On Mon, Jul 5, 2021 at 5:44 AM Martynas Pumputis <m@lambda.lt> wrote: > > 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 <m@lambda.lt> > --- > lib/bpf_libbpf.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c > index d05737a4..f76b90d2 100644 > --- a/lib/bpf_libbpf.c > +++ b/lib/bpf_libbpf.c > @@ -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. > + > /* 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 >
On 7/20/21 10:27 PM, Andrii Nakryiko wrote: > On Mon, Jul 5, 2021 at 5:44 AM Martynas Pumputis <m@lambda.lt> wrote: >> >> 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 <m@lambda.lt> >> --- >> lib/bpf_libbpf.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c >> index d05737a4..f76b90d2 100644 >> --- a/lib/bpf_libbpf.c >> +++ b/lib/bpf_libbpf.c >> @@ -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. 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. > >> + >> /* 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 >>
On 7/21/21 8:47 AM, Martynas Pumputis wrote: >>> diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c >>> index d05737a4..f76b90d2 100644 >>> --- a/lib/bpf_libbpf.c >>> +++ b/lib/bpf_libbpf.c >>> @@ -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. > > 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. 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.
On 7/21/21 4:59 PM, David Ahern wrote: > On 7/21/21 8:47 AM, Martynas Pumputis wrote: >>>> diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c >>>> index d05737a4..f76b90d2 100644 >>>> --- a/lib/bpf_libbpf.c >>>> +++ b/lib/bpf_libbpf.c >>>> @@ -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. >> >> 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. > > 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#
On Wed, Jul 21, 2021 at 8:25 AM Martynas Pumputis <m@lambda.lt> wrote: > > > > On 7/21/21 4:59 PM, David Ahern wrote: > > On 7/21/21 8:47 AM, Martynas Pumputis wrote: > >>>> diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c > >>>> index d05737a4..f76b90d2 100644 > >>>> --- a/lib/bpf_libbpf.c > >>>> +++ b/lib/bpf_libbpf.c > >>>> @@ -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. > >> > >> 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. > > > > 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? > I'm not familiar with those parts of bpftool, I've never used bpftool's command to load BPF programs. Please go check the code. > > [1]: > https://docs.google.com/document/d/1UyjTZuPFWiPFyKk1tV5an11_iaRuec6U-ZESZ54nNTY/edit#
On Wed, Jul 21, 2021 at 04:47:14PM +0200, Martynas Pumputis wrote: > > > diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c > > > index d05737a4..f76b90d2 100644 > > > --- a/lib/bpf_libbpf.c > > > +++ b/lib/bpf_libbpf.c > > > @@ -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. > > 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
On Thu, Jul 22, 2021 at 9:41 PM Hangbin Liu <haliu@redhat.com> wrote: > > On Wed, Jul 21, 2021 at 04:47:14PM +0200, Martynas Pumputis wrote: > > > > diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c > > > > index d05737a4..f76b90d2 100644 > > > > --- a/lib/bpf_libbpf.c > > > > +++ b/lib/bpf_libbpf.c > > > > @@ -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. > > > > 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. 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. > > Thanks > Hangbin >
On Thu, Jul 22, 2021 at 09:51:50PM -0700, Andrii Nakryiko wrote: > > > > 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. > > > > 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. > > 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
On Fri, Jul 23, 2021 at 12:55 AM Hangbin Liu <haliu@redhat.com> wrote: > > On Thu, Jul 22, 2021 at 09:51:50PM -0700, Andrii Nakryiko wrote: > > > > > 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. > > > > > > 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. > > > > 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. > Thanks > Hangbin >
On 7/22/21 10:51 PM, Andrii Nakryiko wrote: > On Thu, Jul 22, 2021 at 9:41 PM Hangbin Liu <haliu@redhat.com> wrote: >> >> On Wed, Jul 21, 2021 at 04:47:14PM +0200, Martynas Pumputis wrote: >>>>> diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c >>>>> index d05737a4..f76b90d2 100644 >>>>> --- a/lib/bpf_libbpf.c >>>>> +++ b/lib/bpf_libbpf.c >>>>> @@ -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. >>> >>> 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. > > 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.
On Fri, Jul 23, 2021 at 5:12 PM David Ahern <dsahern@gmail.com> wrote: > > On 7/22/21 10:51 PM, Andrii Nakryiko wrote: > > On Thu, Jul 22, 2021 at 9:41 PM Hangbin Liu <haliu@redhat.com> wrote: > >> > >> On Wed, Jul 21, 2021 at 04:47:14PM +0200, Martynas Pumputis wrote: > >>>>> diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c > >>>>> index d05737a4..f76b90d2 100644 > >>>>> --- a/lib/bpf_libbpf.c > >>>>> +++ b/lib/bpf_libbpf.c > >>>>> @@ -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. > >>> > >>> 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. > > > > 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. Right. > > 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. > > 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.
On Fri, Jul 23, 2021 at 09:09:01AM -0700, Andrii Nakryiko wrote: > On Fri, Jul 23, 2021 at 12:55 AM Hangbin Liu <haliu@redhat.com> wrote: > > > > On Thu, Jul 22, 2021 at 09:51:50PM -0700, Andrii Nakryiko wrote: > > > > > > 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. > > > > > > > > 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. > > > > > > 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. Cool, we are in the same page now. Thanks Hangbin
On 7/23/21 6:25 PM, Andrii Nakryiko wrote: >>>>>> 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. >>>> >>>> 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. > > Right. > >> >> 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. > 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.
On Mon, Jul 26, 2021 at 6:58 AM David Ahern <dsahern@gmail.com> wrote: > > On 7/23/21 6:25 PM, Andrii Nakryiko wrote: > >>>>>> 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. > >>>> > >>>> 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. > > > > Right. > > > >> > >> 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. > > > > 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.
On 7/26/21 9:13 AM, Andrii Nakryiko wrote: > On Mon, Jul 26, 2021 at 6:58 AM David Ahern <dsahern@gmail.com> wrote: >> >> On 7/23/21 6:25 PM, Andrii Nakryiko wrote: >>>>>>>> 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. >>>>>> >>>>>> 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. >>> >>> Right. >>> >>>> >>>> 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. >>> >> >> 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. > 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.
On Mon, Jul 26, 2021 at 7:51 PM David Ahern <dsahern@gmail.com> wrote: > > On 7/26/21 9:13 AM, Andrii Nakryiko wrote: > > On Mon, Jul 26, 2021 at 6:58 AM David Ahern <dsahern@gmail.com> wrote: > >> > >> On 7/23/21 6:25 PM, Andrii Nakryiko wrote: > >>>>>>>> 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. > >>>>>> > >>>>>> 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. > >>> > >>> Right. > >>> > >>>> > >>>> 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. > >>> > >> > >> 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. > > > > 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 Ok, I see, that's the problem with pinning path using section name as an identifier (same wrong assumption made a long time ago). We have a task to fix that ([0]) and use program name instead of section name for this, but it's a backwards incompatible change, so users will have to opt-in. And we should fix bpftool as well, of course, though I never used bpftool to load programs so I wasn't even aware it's doing pinning like that. [0] https://github.com/libbpf/libbpf/issues/273 > > 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. > And I didn't lie, see progs/test_check_mtu.c as an example, it has 6 XDP programs with SEC("xdp"). The problem is in the pinning, which is in general an area that was pretty ad-hoc and not very well thought out in libbpf, growing organically. This hopefully will be addressed and improved before libbpf 1.0 is finalized. Right now users can override the default pin path by setting it explicitly with bpf_program__set_pin_path(), which might be a good idea to do for this new "prog" approach that Hangbin proposed. > 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. >
diff --git a/lib/bpf_libbpf.c b/lib/bpf_libbpf.c index d05737a4..f76b90d2 100644 --- a/lib/bpf_libbpf.c +++ b/lib/bpf_libbpf.c @@ -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; }
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 <m@lambda.lt> --- lib/bpf_libbpf.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)