[PATCH iproute2] libbpf: fix attach of prog with multiple sections

[PATCH iproute2] libbpf: fix attach of prog with multiple sections

[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 <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

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Hangbin Liu]

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>

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Andrii Nakryiko]

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
>

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Martynas Pumputis]

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
>>

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[David Ahern]

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.

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Martynas Pumputis]

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#

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Andrii Nakryiko]

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#

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Hangbin Liu]

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

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Andrii Nakryiko]

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
>

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Hangbin Liu]

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

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Andrii Nakryiko]

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
>

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Hangbin Liu]

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

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[David Ahern]

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.

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Andrii Nakryiko]

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.

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[David Ahern]

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.

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Andrii Nakryiko]

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.

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[David Ahern]

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.

Re: [PATCH iproute2] libbpf: fix attach of prog with multiple sections

[Andrii Nakryiko]

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.
>