Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs

[Andrii Nakryiko]

On Wed, Aug 5, 2020 at 12:23 AM Song Liu <songliubraving@xxxxxx> wrote:
>
>
>
> > On Aug 4, 2020, at 11:54 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Aug 4, 2020 at 11:26 PM Song Liu <songliubraving@xxxxxx> wrote:
> >>
> >>
> >>
> >>> On Aug 4, 2020, at 10:32 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> >>>
> >>> On Tue, Aug 4, 2020 at 8:59 PM Song Liu <songliubraving@xxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> >>>>>
> >>>>> On Mon, Aug 3, 2020 at 6:18 PM Song Liu <songliubraving@xxxxxx> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>> On Sat, Aug 1, 2020 at 1:50 AM Song Liu <songliubraving@xxxxxx> 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 "userblah", I'm sorry.
>
> If the concern with "userblah" is real and unbearable, so is "xdpblah"
> and "perf_eventblah", and so on, and so on.
>

Oh yeah, "xdpblah" makes me cringe. "Some other kid is doing wrong
thing, let me do it as well" style of argument never worked for me
with my parents, I don't see why it should work here :)

But anyways, let's table this discussion, it's not worth spending so
much time on it. As I said, I'm going to start enforcing standardized
separator or a single program type word soon enough. Might as well do
that for "userblah".

> >
> >>
> >>> 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?
> >
> > Because libbpf is dumb in that regard today? And instead of migrating
> > users later, I want to prevent users making bad choices right now.
>
> The good choice here is to use "user", IMO. And you are preventing people
> to use it. If user has to use "user/" for now. They will have to update
> the programs later, right? If the conclusion is "user/xxx" is the ultimate
> goal, I would agree with "user/" for now.

They won't have to update, "user/something" would still work.

>
> > Then relax it, if necessary. Alternatively, we can fix up libbpf logic
> > before the USER program type lands.
>
> I don't see why the USER program type need to wait for libbpf fix, as
> "xdp", "perf_event", etc. all work well today.

It's about being a good citizen and helping move libbpf forward.

>
> >
> >> 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...
> >
> > That's also bad, and again, we can make libbpf better. I think moving
> > forward any non-recognized BPF program type should be reported by
> > libbpf as an error. But we can't do it right now, we have to have a
> > period in which users will get a chance to update their BPF programs.
> > This will have to happen over few libbpf releases at least. So please
> > join in on the fun of fixing stuff like this.
>
> I'd love to join the fun. Maybe after user program lands ;)
>
>

Of course :) see above.