Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
On Wed, Aug 5, 2020 at 12:23 AM Song Liu wrote:
>
>
>
> > On Aug 4, 2020, at 11:54 PM, Andrii Nakryiko
> > wrote:
> >
> > On Tue, Aug 4, 2020 at 11:26 PM Song Liu wrote:
> >>
> >>
> >>
> >>> On Aug 4, 2020, at 10:32 PM, Andrii Nakryiko
> >>> wrote:
> >>>
> >>> On Tue, Aug 4, 2020 at 8:59 PM Song Liu wrote:
>
>
>
> > On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko
> > wrote:
> >
> > On Mon, Aug 3, 2020 at 6:18 PM Song Liu wrote:
> >>
> >>
> >>
> >>> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
> >>> wrote:
> >>>
> >>> On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>
> >>
> >> [...]
> >>
> >>>
> };
>
> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
> *test_attr);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b9f11f854985b..9ce175a486214 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[]
> = {
> BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
> BPF_PROG_SEC("lwt_seg6local",
> BPF_PROG_TYPE_LWT_SEG6LOCAL),
> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
> >>>
> >>> let's do "user/" for consistency with most other prog types (and nice
> >>> separation between prog type and custom user name)
> >>
> >> About "user" vs. "user/", I still think "user" is better.
> >>
> >> Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
> >> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
> >> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and
> >> "user/xxx"
> >> would also work. However, if we specify "user/" here, programs that
> >> used
> >> "user" by accident will fail to load, with a message like:
> >>
> >> libbpf: failed to load program 'user'
> >>
> >> which is confusing.
> >
> > xdp, perf_event and a bunch of others don't enforce it, that's true,
> > they are a bit of a legacy,
>
> I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just uses
> "struct_ops".
>
> > unfortunately. But all the recent ones do,
> > and we explicitly did that for xdp_dev/xdp_cpu, for instance.
> > Specifying just "user" in the spec would allow something nonsensical
> > like "userargh", for instance, due to this being treated as a prefix.
> > There is no harm to require users to do "user/my_prog", though.
>
> I don't see why allowing "userargh" is a problem. Failing "user" is
> more confusing. We can probably improve that by a hint like:
>
> libbpf: failed to load program 'user', do you mean "user/"?
>
> But it is pretty silly. "user/something_never_used" also looks weird.
> >>>
> >>> "userargh" is terrible, IMO. It's a different identifier that just
> >>> happens to have the first 4 letters matching "user" program type.
> >>> There must be either a standardized separator (which happens to be
> >>> '/') or none. See the suggestion below.
> >>
> >> We have no problem deal with "a different identifier that just happens
> >> to have the first letters matching", like xdp vs. xdp_devmap and
> >> xdp_cpumap, right?
> >>
> >
> > xdp vs xdp_devmap is an entirely different thing. We deal with it by
> > checking xdp_devmap first. What I'm saying is that user can do
> > "xdpomg" and libbpf would be happy (today). And I don't think that's
> > good. But further, if someone does something like "xdp_devmap_omg",
> > guess which program type will be inferred? Hint: not xdp_devmap and
> > libbpf won't report an error either. All because "xdp" is so lax
> > today.
> >
>
> > Alternatively, we could introduce a new convention in the spec,
> > something like "user?", which would accept either "user" or
> > "user/something", but not "user/" nor "userblah". We can try that as
> > well.
>
> Again, I don't really understand why allowing "userblah" is a problem.
> We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
> fine so far.
> >>>
> >>> Right, we have "xdp_devmap/" and "xdp_cpumap/", as you say. I haven't
> >>> seen so much pushback against trailing forward slash with those ;)
> >>
> >> I haven't seen any issue with old "perf_event", "xdp" and new "struct_ops"
> >> either.
> >>
> >>>
> >>> But anyways, as part of deprecating APIs and preparing libbpf for 1.0
> >>> release over this half, I think I'm going to emit warnings for names
> >>> like "prog_type_whatever" or "prog_typeevenworse", etc. And asking
> >>> users to normalize section names to either "prog_type" or
> >>>
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
> On Aug 4, 2020, at 11:54 PM, Andrii Nakryiko
> wrote:
>
> On Tue, Aug 4, 2020 at 11:26 PM Song Liu wrote:
>>
>>
>>
>>> On Aug 4, 2020, at 10:32 PM, Andrii Nakryiko
>>> wrote:
>>>
>>> On Tue, Aug 4, 2020 at 8:59 PM Song Liu wrote:
> On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko
> wrote:
>
> On Mon, Aug 3, 2020 at 6:18 PM Song Liu wrote:
>>
>>
>>
>>> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
>>> wrote:
>>>
>>> On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>>
>> [...]
>>
>>>
};
LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
*test_attr);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b9f11f854985b..9ce175a486214 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] =
{
BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
BPF_PROG_SEC("lwt_seg6local",
BPF_PROG_TYPE_LWT_SEG6LOCAL),
+ BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
>>>
>>> let's do "user/" for consistency with most other prog types (and nice
>>> separation between prog type and custom user name)
>>
>> About "user" vs. "user/", I still think "user" is better.
>>
>> Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
>> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
>> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
>> would also work. However, if we specify "user/" here, programs that used
>> "user" by accident will fail to load, with a message like:
>>
>> libbpf: failed to load program 'user'
>>
>> which is confusing.
>
> xdp, perf_event and a bunch of others don't enforce it, that's true,
> they are a bit of a legacy,
I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just uses
"struct_ops".
> unfortunately. But all the recent ones do,
> and we explicitly did that for xdp_dev/xdp_cpu, for instance.
> Specifying just "user" in the spec would allow something nonsensical
> like "userargh", for instance, due to this being treated as a prefix.
> There is no harm to require users to do "user/my_prog", though.
I don't see why allowing "userargh" is a problem. Failing "user" is
more confusing. We can probably improve that by a hint like:
libbpf: failed to load program 'user', do you mean "user/"?
But it is pretty silly. "user/something_never_used" also looks weird.
>>>
>>> "userargh" is terrible, IMO. It's a different identifier that just
>>> happens to have the first 4 letters matching "user" program type.
>>> There must be either a standardized separator (which happens to be
>>> '/') or none. See the suggestion below.
>>
>> We have no problem deal with "a different identifier that just happens
>> to have the first letters matching", like xdp vs. xdp_devmap and
>> xdp_cpumap, right?
>>
>
> xdp vs xdp_devmap is an entirely different thing. We deal with it by
> checking xdp_devmap first. What I'm saying is that user can do
> "xdpomg" and libbpf would be happy (today). And I don't think that's
> good. But further, if someone does something like "xdp_devmap_omg",
> guess which program type will be inferred? Hint: not xdp_devmap and
> libbpf won't report an error either. All because "xdp" is so lax
> today.
>
> Alternatively, we could introduce a new convention in the spec,
> something like "user?", which would accept either "user" or
> "user/something", but not "user/" nor "userblah". We can try that as
> well.
Again, I don't really understand why allowing "userblah" is a problem.
We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
fine so far.
>>>
>>> Right, we have "xdp_devmap/" and "xdp_cpumap/", as you say. I haven't
>>> seen so much pushback against trailing forward slash with those ;)
>>
>> I haven't seen any issue with old "perf_event", "xdp" and new "struct_ops"
>> either.
>>
>>>
>>> But anyways, as part of deprecating APIs and preparing libbpf for 1.0
>>> release over this half, I think I'm going to emit warnings for names
>>> like "prog_type_whatever" or "prog_typeevenworse", etc. And asking
>>> users to normalize section names to either "prog_type" or
>>> "prog_type/something/here", whichever makes sense for a specific
>>> program type.
>>
>> Exactly, "user" makes sense here; while "kprobe/__set_task_comm" makes
>> sense for kprobe.
>
> Right, but "userblah" doesn't. It would be great if you could help
> make what I described above become
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
On Tue, Aug 4, 2020 at 11:26 PM Song Liu wrote:
>
>
>
> > On Aug 4, 2020, at 10:32 PM, Andrii Nakryiko
> > wrote:
> >
> > On Tue, Aug 4, 2020 at 8:59 PM Song Liu wrote:
> >>
> >>
> >>
> >>> On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko
> >>> wrote:
> >>>
> >>> On Mon, Aug 3, 2020 at 6:18 PM Song Liu wrote:
>
>
>
> > On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
> > wrote:
> >
> > On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
> >>
>
> [...]
>
> >
> >> };
> >>
> >> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
> >> *test_attr);
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index b9f11f854985b..9ce175a486214 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] =
> >> {
> >> BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> >> BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
> >> BPF_PROG_SEC("lwt_seg6local",
> >> BPF_PROG_TYPE_LWT_SEG6LOCAL),
> >> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
> >
> > let's do "user/" for consistency with most other prog types (and nice
> > separation between prog type and custom user name)
>
> About "user" vs. "user/", I still think "user" is better.
>
> Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
> would also work. However, if we specify "user/" here, programs that used
> "user" by accident will fail to load, with a message like:
>
> libbpf: failed to load program 'user'
>
> which is confusing.
> >>>
> >>> xdp, perf_event and a bunch of others don't enforce it, that's true,
> >>> they are a bit of a legacy,
> >>
> >> I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just uses
> >> "struct_ops".
> >>
> >>> unfortunately. But all the recent ones do,
> >>> and we explicitly did that for xdp_dev/xdp_cpu, for instance.
> >>> Specifying just "user" in the spec would allow something nonsensical
> >>> like "userargh", for instance, due to this being treated as a prefix.
> >>> There is no harm to require users to do "user/my_prog", though.
> >>
> >> I don't see why allowing "userargh" is a problem. Failing "user" is
> >> more confusing. We can probably improve that by a hint like:
> >>
> >>libbpf: failed to load program 'user', do you mean "user/"?
> >>
> >> But it is pretty silly. "user/something_never_used" also looks weird.
> >
> > "userargh" is terrible, IMO. It's a different identifier that just
> > happens to have the first 4 letters matching "user" program type.
> > There must be either a standardized separator (which happens to be
> > '/') or none. See the suggestion below.
>
> We have no problem deal with "a different identifier that just happens
> to have the first letters matching", like xdp vs. xdp_devmap and
> xdp_cpumap, right?
>
xdp vs xdp_devmap is an entirely different thing. We deal with it by
checking xdp_devmap first. What I'm saying is that user can do
"xdpomg" and libbpf would be happy (today). And I don't think that's
good. But further, if someone does something like "xdp_devmap_omg",
guess which program type will be inferred? Hint: not xdp_devmap and
libbpf won't report an error either. All because "xdp" is so lax
today.
> >>
> >>> Alternatively, we could introduce a new convention in the spec,
> >>> something like "user?", which would accept either "user" or
> >>> "user/something", but not "user/" nor "userblah". We can try that as
> >>> well.
> >>
> >> Again, I don't really understand why allowing "userblah" is a problem.
> >> We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
> >> fine so far.
> >
> > Right, we have "xdp_devmap/" and "xdp_cpumap/", as you say. I haven't
> > seen so much pushback against trailing forward slash with those ;)
>
> I haven't seen any issue with old "perf_event", "xdp" and new "struct_ops"
> either.
>
> >
> > But anyways, as part of deprecating APIs and preparing libbpf for 1.0
> > release over this half, I think I'm going to emit warnings for names
> > like "prog_type_whatever" or "prog_typeevenworse", etc. And asking
> > users to normalize section names to either "prog_type" or
> > "prog_type/something/here", whichever makes sense for a specific
> > program type.
>
> Exactly, "user" makes sense here; while "kprobe/__set_task_comm" makes
> sense for kprobe.
Right, but "userblah" doesn't. It would be great if you could help
make what I described above become true. But at least don't make it
worse by allowing unrestricted "user" prefix. I'm OK with strict
"user" or "user/blah", I'm not OK with
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
> On Aug 4, 2020, at 10:32 PM, Andrii Nakryiko
> wrote:
>
> On Tue, Aug 4, 2020 at 8:59 PM Song Liu wrote:
>>
>>
>>
>>> On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko
>>> wrote:
>>>
>>> On Mon, Aug 3, 2020 at 6:18 PM Song Liu wrote:
> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
> wrote:
>
> On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>>
[...]
>
>> };
>>
>> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
>> *test_attr);
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index b9f11f854985b..9ce175a486214 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
>> BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
>> BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
>> BPF_PROG_SEC("lwt_seg6local",
>> BPF_PROG_TYPE_LWT_SEG6LOCAL),
>> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
>
> let's do "user/" for consistency with most other prog types (and nice
> separation between prog type and custom user name)
About "user" vs. "user/", I still think "user" is better.
Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
would also work. However, if we specify "user/" here, programs that used
"user" by accident will fail to load, with a message like:
libbpf: failed to load program 'user'
which is confusing.
>>>
>>> xdp, perf_event and a bunch of others don't enforce it, that's true,
>>> they are a bit of a legacy,
>>
>> I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just uses
>> "struct_ops".
>>
>>> unfortunately. But all the recent ones do,
>>> and we explicitly did that for xdp_dev/xdp_cpu, for instance.
>>> Specifying just "user" in the spec would allow something nonsensical
>>> like "userargh", for instance, due to this being treated as a prefix.
>>> There is no harm to require users to do "user/my_prog", though.
>>
>> I don't see why allowing "userargh" is a problem. Failing "user" is
>> more confusing. We can probably improve that by a hint like:
>>
>>libbpf: failed to load program 'user', do you mean "user/"?
>>
>> But it is pretty silly. "user/something_never_used" also looks weird.
>
> "userargh" is terrible, IMO. It's a different identifier that just
> happens to have the first 4 letters matching "user" program type.
> There must be either a standardized separator (which happens to be
> '/') or none. See the suggestion below.
We have no problem deal with "a different identifier that just happens
to have the first letters matching", like xdp vs. xdp_devmap and
xdp_cpumap, right?
>>
>>> Alternatively, we could introduce a new convention in the spec,
>>> something like "user?", which would accept either "user" or
>>> "user/something", but not "user/" nor "userblah". We can try that as
>>> well.
>>
>> Again, I don't really understand why allowing "userblah" is a problem.
>> We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
>> fine so far.
>
> Right, we have "xdp_devmap/" and "xdp_cpumap/", as you say. I haven't
> seen so much pushback against trailing forward slash with those ;)
I haven't seen any issue with old "perf_event", "xdp" and new "struct_ops"
either.
>
> But anyways, as part of deprecating APIs and preparing libbpf for 1.0
> release over this half, I think I'm going to emit warnings for names
> like "prog_type_whatever" or "prog_typeevenworse", etc. And asking
> users to normalize section names to either "prog_type" or
> "prog_type/something/here", whichever makes sense for a specific
> program type.
Exactly, "user" makes sense here; while "kprobe/__set_task_comm" makes
sense for kprobe.
> Right now libbpf doesn't allow two separate BPF programs
> with the same section name, so enforcing strict "user" is limiting to
> users. We are going to lift that restriction pretty soon, though. But
> for now, please stick with what we've been doing lately and mark it as
> "user/", later we'll allow just "user" as well.
Since we would allow "user" later, why we have to reject it for now?
Imagine the user just compiled and booted into a new kernel with user
program support; and then got the following message:
libbpf: failed to load program 'user'
If I were the user, I would definitely question whether the kernel was
correct...
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
On Tue, Aug 4, 2020 at 8:59 PM Song Liu wrote:
>
>
>
> > On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko
> > wrote:
> >
> > On Mon, Aug 3, 2020 at 6:18 PM Song Liu wrote:
> >>
> >>
> >>
> >>> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
> >>> wrote:
> >>>
> >>> On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>
> >>
> >> [...]
> >>
> >>>
> };
>
> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
> *test_attr);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b9f11f854985b..9ce175a486214 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
> BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
> BPF_PROG_SEC("lwt_seg6local",
> BPF_PROG_TYPE_LWT_SEG6LOCAL),
> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
> >>>
> >>> let's do "user/" for consistency with most other prog types (and nice
> >>> separation between prog type and custom user name)
> >>
> >> About "user" vs. "user/", I still think "user" is better.
> >>
> >> Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
> >> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
> >> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
> >> would also work. However, if we specify "user/" here, programs that used
> >> "user" by accident will fail to load, with a message like:
> >>
> >>libbpf: failed to load program 'user'
> >>
> >> which is confusing.
> >
> > xdp, perf_event and a bunch of others don't enforce it, that's true,
> > they are a bit of a legacy,
>
> I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just uses
> "struct_ops".
>
> > unfortunately. But all the recent ones do,
> > and we explicitly did that for xdp_dev/xdp_cpu, for instance.
> > Specifying just "user" in the spec would allow something nonsensical
> > like "userargh", for instance, due to this being treated as a prefix.
> > There is no harm to require users to do "user/my_prog", though.
>
> I don't see why allowing "userargh" is a problem. Failing "user" is
> more confusing. We can probably improve that by a hint like:
>
> libbpf: failed to load program 'user', do you mean "user/"?
>
> But it is pretty silly. "user/something_never_used" also looks weird.
"userargh" is terrible, IMO. It's a different identifier that just
happens to have the first 4 letters matching "user" program type.
There must be either a standardized separator (which happens to be
'/') or none. See the suggestion below.
>
> > Alternatively, we could introduce a new convention in the spec,
> > something like "user?", which would accept either "user" or
> > "user/something", but not "user/" nor "userblah". We can try that as
> > well.
>
> Again, I don't really understand why allowing "userblah" is a problem.
> We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
> fine so far.
Right, we have "xdp_devmap/" and "xdp_cpumap/", as you say. I haven't
seen so much pushback against trailing forward slash with those ;)
But anyways, as part of deprecating APIs and preparing libbpf for 1.0
release over this half, I think I'm going to emit warnings for names
like "prog_type_whatever" or "prog_typeevenworse", etc. And asking
users to normalize section names to either "prog_type" or
"prog_type/something/here", whichever makes sense for a specific
program type. Right now libbpf doesn't allow two separate BPF programs
with the same section name, so enforcing strict "user" is limiting to
users. We are going to lift that restriction pretty soon, though. But
for now, please stick with what we've been doing lately and mark it as
"user/", later we'll allow just "user" as well.
>
> Thanks,
> Song
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
> On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko wrote:
>
> On Mon, Aug 3, 2020 at 6:18 PM Song Liu wrote:
>>
>>
>>
>>> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
>>> wrote:
>>>
>>> On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>>
>> [...]
>>
>>>
};
LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
*test_attr);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b9f11f854985b..9ce175a486214 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
+ BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
>>>
>>> let's do "user/" for consistency with most other prog types (and nice
>>> separation between prog type and custom user name)
>>
>> About "user" vs. "user/", I still think "user" is better.
>>
>> Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
>> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
>> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
>> would also work. However, if we specify "user/" here, programs that used
>> "user" by accident will fail to load, with a message like:
>>
>>libbpf: failed to load program 'user'
>>
>> which is confusing.
>
> xdp, perf_event and a bunch of others don't enforce it, that's true,
> they are a bit of a legacy,
I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just uses
"struct_ops".
> unfortunately. But all the recent ones do,
> and we explicitly did that for xdp_dev/xdp_cpu, for instance.
> Specifying just "user" in the spec would allow something nonsensical
> like "userargh", for instance, due to this being treated as a prefix.
> There is no harm to require users to do "user/my_prog", though.
I don't see why allowing "userargh" is a problem. Failing "user" is
more confusing. We can probably improve that by a hint like:
libbpf: failed to load program 'user', do you mean "user/"?
But it is pretty silly. "user/something_never_used" also looks weird.
> Alternatively, we could introduce a new convention in the spec,
> something like "user?", which would accept either "user" or
> "user/something", but not "user/" nor "userblah". We can try that as
> well.
Again, I don't really understand why allowing "userblah" is a problem.
We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
fine so far.
Thanks,
Song
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
On Mon, Aug 3, 2020 at 6:18 PM Song Liu wrote:
>
>
>
> > On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
> > wrote:
> >
> > On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
> >>
>
> [...]
>
> >
> >> };
> >>
> >> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
> >> *test_attr);
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index b9f11f854985b..9ce175a486214 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
> >>BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> >>BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
> >>BPF_PROG_SEC("lwt_seg6local",
> >> BPF_PROG_TYPE_LWT_SEG6LOCAL),
> >> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
> >
> > let's do "user/" for consistency with most other prog types (and nice
> > separation between prog type and custom user name)
>
> About "user" vs. "user/", I still think "user" is better.
>
> Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
> would also work. However, if we specify "user/" here, programs that used
> "user" by accident will fail to load, with a message like:
>
> libbpf: failed to load program 'user'
>
> which is confusing.
xdp, perf_event and a bunch of others don't enforce it, that's true,
they are a bit of a legacy, unfortunately. But all the recent ones do,
and we explicitly did that for xdp_dev/xdp_cpu, for instance.
Specifying just "user" in the spec would allow something nonsensical
like "userargh", for instance, due to this being treated as a prefix.
There is no harm to require users to do "user/my_prog", though.
Alternatively, we could introduce a new convention in the spec,
something like "user?", which would accept either "user" or
"user/something", but not "user/" nor "userblah". We can try that as
well.
>
> Thanks,
> Song
>
> [...]
>
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko wrote:
>
> On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>>
[...]
>
>> };
>>
>> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
>> *test_attr);
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index b9f11f854985b..9ce175a486214 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
>>BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
>>BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
>>BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
>> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
>
> let's do "user/" for consistency with most other prog types (and nice
> separation between prog type and custom user name)
About "user" vs. "user/", I still think "user" is better.
Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
would also work. However, if we specify "user/" here, programs that used
"user" by accident will fail to load, with a message like:
libbpf: failed to load program 'user'
which is confusing.
Thanks,
Song
[...]
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
On Sun, Aug 2, 2020 at 9:21 PM Song Liu wrote:
>
>
>
> > On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko
> > wrote:
> >
> > On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
> >>
> >> Add cpu_plus to bpf_prog_test_run_attr. Add BPF_PROG_SEC "user" for
> >> BPF_PROG_TYPE_USER programs.
> >>
> >> Signed-off-by: Song Liu
> >> ---
> >> tools/lib/bpf/bpf.c | 1 +
> >> tools/lib/bpf/bpf.h | 3 +++
> >> tools/lib/bpf/libbpf.c| 1 +
> >> tools/lib/bpf/libbpf_probes.c | 1 +
> >> 4 files changed, 6 insertions(+)
> >>
> >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> >> index e1bdf214f75fe..b28c3daa9c270 100644
> >> --- a/tools/lib/bpf/bpf.c
> >> +++ b/tools/lib/bpf/bpf.c
> >> @@ -693,6 +693,7 @@ int bpf_prog_test_run_xattr(struct
> >> bpf_prog_test_run_attr *test_attr)
> >>attr.test.ctx_size_in = test_attr->ctx_size_in;
> >>attr.test.ctx_size_out = test_attr->ctx_size_out;
> >>attr.test.repeat = test_attr->repeat;
> >> + attr.test.cpu_plus = test_attr->cpu_plus;
> >>
> >>ret = sys_bpf(BPF_PROG_TEST_RUN, , sizeof(attr));
> >>test_attr->data_size_out = attr.test.data_size_out;
> >> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> >> index 6d367e01d05e9..0c799740df566 100644
> >> --- a/tools/lib/bpf/bpf.h
> >> +++ b/tools/lib/bpf/bpf.h
> >> @@ -205,6 +205,9 @@ struct bpf_prog_test_run_attr {
> >>void *ctx_out; /* optional */
> >>__u32 ctx_size_out; /* in: max length of ctx_out
> >> * out: length of cxt_out */
> >> + __u32 cpu_plus; /* specify which cpu to run the test with
> >> +* cpu_plus = cpu_id + 1.
> >> +* If cpu_plus = 0, run on current cpu */
> >
> > We can't do this due to ABI guarantees. We'll have to add a new API
> > using OPTS arguments.
>
> To make sure I understand this correctly, the concern is when we compile
> the binary with one version of libbpf and run it with libbpf.so of a
> different version, right?
>
yep, exactly
> Thanks,
> Song
>
> >
> >> };
> >>
> >> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
> >> *test_attr);
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index b9f11f854985b..9ce175a486214 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
> >>BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> >>BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
> >>BPF_PROG_SEC("lwt_seg6local",
> >> BPF_PROG_TYPE_LWT_SEG6LOCAL),
> >> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
> >
> > let's do "user/" for consistency with most other prog types (and nice
> > separation between prog type and custom user name)
>
> Will update.
>
thanks!
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko wrote:
>
> On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>>
>> Add cpu_plus to bpf_prog_test_run_attr. Add BPF_PROG_SEC "user" for
>> BPF_PROG_TYPE_USER programs.
>>
>> Signed-off-by: Song Liu
>> ---
>> tools/lib/bpf/bpf.c | 1 +
>> tools/lib/bpf/bpf.h | 3 +++
>> tools/lib/bpf/libbpf.c| 1 +
>> tools/lib/bpf/libbpf_probes.c | 1 +
>> 4 files changed, 6 insertions(+)
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index e1bdf214f75fe..b28c3daa9c270 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -693,6 +693,7 @@ int bpf_prog_test_run_xattr(struct
>> bpf_prog_test_run_attr *test_attr)
>>attr.test.ctx_size_in = test_attr->ctx_size_in;
>>attr.test.ctx_size_out = test_attr->ctx_size_out;
>>attr.test.repeat = test_attr->repeat;
>> + attr.test.cpu_plus = test_attr->cpu_plus;
>>
>>ret = sys_bpf(BPF_PROG_TEST_RUN, , sizeof(attr));
>>test_attr->data_size_out = attr.test.data_size_out;
>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> index 6d367e01d05e9..0c799740df566 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
>> @@ -205,6 +205,9 @@ struct bpf_prog_test_run_attr {
>>void *ctx_out; /* optional */
>>__u32 ctx_size_out; /* in: max length of ctx_out
>> * out: length of cxt_out */
>> + __u32 cpu_plus; /* specify which cpu to run the test with
>> +* cpu_plus = cpu_id + 1.
>> +* If cpu_plus = 0, run on current cpu */
>
> We can't do this due to ABI guarantees. We'll have to add a new API
> using OPTS arguments.
To make sure I understand this correctly, the concern is when we compile
the binary with one version of libbpf and run it with libbpf.so of a
different version, right?
Thanks,
Song
>
>> };
>>
>> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
>> *test_attr);
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index b9f11f854985b..9ce175a486214 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
>>BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
>>BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
>>BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
>> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
>
> let's do "user/" for consistency with most other prog types (and nice
> separation between prog type and custom user name)
Will update.
Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
On Sat, Aug 1, 2020 at 1:50 AM Song Liu wrote:
>
> Add cpu_plus to bpf_prog_test_run_attr. Add BPF_PROG_SEC "user" for
> BPF_PROG_TYPE_USER programs.
>
> Signed-off-by: Song Liu
> ---
> tools/lib/bpf/bpf.c | 1 +
> tools/lib/bpf/bpf.h | 3 +++
> tools/lib/bpf/libbpf.c| 1 +
> tools/lib/bpf/libbpf_probes.c | 1 +
> 4 files changed, 6 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index e1bdf214f75fe..b28c3daa9c270 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -693,6 +693,7 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
> *test_attr)
> attr.test.ctx_size_in = test_attr->ctx_size_in;
> attr.test.ctx_size_out = test_attr->ctx_size_out;
> attr.test.repeat = test_attr->repeat;
> + attr.test.cpu_plus = test_attr->cpu_plus;
>
> ret = sys_bpf(BPF_PROG_TEST_RUN, , sizeof(attr));
> test_attr->data_size_out = attr.test.data_size_out;
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 6d367e01d05e9..0c799740df566 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -205,6 +205,9 @@ struct bpf_prog_test_run_attr {
> void *ctx_out; /* optional */
> __u32 ctx_size_out; /* in: max length of ctx_out
> * out: length of cxt_out */
> + __u32 cpu_plus; /* specify which cpu to run the test with
> +* cpu_plus = cpu_id + 1.
> +* If cpu_plus = 0, run on current cpu */
We can't do this due to ABI guarantees. We'll have to add a new API
using OPTS arguments.
> };
>
> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
> *test_attr);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b9f11f854985b..9ce175a486214 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
> BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
> BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
> + BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
let's do "user/" for consistency with most other prog types (and nice
separation between prog type and custom user name)
> BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB,
> BPF_CGROUP_INET_INGRESS),
> BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> index 5a3d3f0784081..163013084000e 100644
> --- a/tools/lib/bpf/libbpf_probes.c
> +++ b/tools/lib/bpf/libbpf_probes.c
> @@ -112,6 +112,7 @@ probe_load(enum bpf_prog_type prog_type, const struct
> bpf_insn *insns,
> case BPF_PROG_TYPE_STRUCT_OPS:
> case BPF_PROG_TYPE_EXT:
> case BPF_PROG_TYPE_LSM:
> + case BPF_PROG_TYPE_USER:
> default:
> break;
> }
> --
> 2.24.1
>
[PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
Add cpu_plus to bpf_prog_test_run_attr. Add BPF_PROG_SEC "user" for
BPF_PROG_TYPE_USER programs.
Signed-off-by: Song Liu
---
tools/lib/bpf/bpf.c | 1 +
tools/lib/bpf/bpf.h | 3 +++
tools/lib/bpf/libbpf.c| 1 +
tools/lib/bpf/libbpf_probes.c | 1 +
4 files changed, 6 insertions(+)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index e1bdf214f75fe..b28c3daa9c270 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -693,6 +693,7 @@ int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
*test_attr)
attr.test.ctx_size_in = test_attr->ctx_size_in;
attr.test.ctx_size_out = test_attr->ctx_size_out;
attr.test.repeat = test_attr->repeat;
+ attr.test.cpu_plus = test_attr->cpu_plus;
ret = sys_bpf(BPF_PROG_TEST_RUN, , sizeof(attr));
test_attr->data_size_out = attr.test.data_size_out;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 6d367e01d05e9..0c799740df566 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -205,6 +205,9 @@ struct bpf_prog_test_run_attr {
void *ctx_out; /* optional */
__u32 ctx_size_out; /* in: max length of ctx_out
* out: length of cxt_out */
+ __u32 cpu_plus; /* specify which cpu to run the test with
+* cpu_plus = cpu_id + 1.
+* If cpu_plus = 0, run on current cpu */
};
LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr
*test_attr);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b9f11f854985b..9ce175a486214 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
BPF_PROG_SEC("lwt_xmit",BPF_PROG_TYPE_LWT_XMIT),
BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
+ BPF_PROG_SEC("user",BPF_PROG_TYPE_USER),
BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB,
BPF_CGROUP_INET_INGRESS),
BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index 5a3d3f0784081..163013084000e 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -112,6 +112,7 @@ probe_load(enum bpf_prog_type prog_type, const struct
bpf_insn *insns,
case BPF_PROG_TYPE_STRUCT_OPS:
case BPF_PROG_TYPE_EXT:
case BPF_PROG_TYPE_LSM:
+ case BPF_PROG_TYPE_USER:
default:
break;
}
--
2.24.1
