| Message ID | 20220331154555.422506-4-milan@mdaverde.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7b53eaa656c34d5b521e245cbfc3aee2d7b89eac |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf/bpftool: add program & link type names | expand |
2022-03-31 11:45 UTC-0400 ~ Milan Landaverde <milan@mdaverde.com> > Previously [1], we were using bpf_probe_prog_type which returned a > bool, but the new libbpf_probe_bpf_prog_type can return a negative > error code on failure. This change decides for bpftool to declare > a program type is not available on probe failure. > > [1] https://lore.kernel.org/bpf/20220202225916.3313522-3-andrii@kernel.org/ > > Signed-off-by: Milan Landaverde <milan@mdaverde.com> > --- > tools/bpf/bpftool/feature.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c > index c2f43a5d38e0..b2fbaa7a6b15 100644 > --- a/tools/bpf/bpftool/feature.c > +++ b/tools/bpf/bpftool/feature.c > @@ -564,7 +564,7 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types, > > res = probe_prog_type_ifindex(prog_type, ifindex); > } else { > - res = libbpf_probe_bpf_prog_type(prog_type, NULL); > + res = libbpf_probe_bpf_prog_type(prog_type, NULL) > 0; > } > > #ifdef USE_LIBCAP Reviewed-by: Quentin Monnet <quentin@isovalent.com> Thanks!
On Fri, Apr 1, 2022 at 9:05 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2022-03-31 11:45 UTC-0400 ~ Milan Landaverde <milan@mdaverde.com> > > Previously [1], we were using bpf_probe_prog_type which returned a > > bool, but the new libbpf_probe_bpf_prog_type can return a negative > > error code on failure. This change decides for bpftool to declare > > a program type is not available on probe failure. > > > > [1] https://lore.kernel.org/bpf/20220202225916.3313522-3-andrii@kernel.org/ > > > > Signed-off-by: Milan Landaverde <milan@mdaverde.com> > > --- > > tools/bpf/bpftool/feature.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c > > index c2f43a5d38e0..b2fbaa7a6b15 100644 > > --- a/tools/bpf/bpftool/feature.c > > +++ b/tools/bpf/bpftool/feature.c > > @@ -564,7 +564,7 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types, > > > > res = probe_prog_type_ifindex(prog_type, ifindex); > > } else { > > - res = libbpf_probe_bpf_prog_type(prog_type, NULL); > > + res = libbpf_probe_bpf_prog_type(prog_type, NULL) > 0; > > } > > > > #ifdef USE_LIBCAP > A completely unrelated question to you, Quentin. How hard is bpftool's dependency on libcap? We've recently removed libcap from selftests, I wonder if it would be possible to do that for bpftool as well to reduce amount of shared libraries bpftool depends on. > Reviewed-by: Quentin Monnet <quentin@isovalent.com> > > Thanks!
On Fri, 1 Apr 2022 at 19:42, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Apr 1, 2022 at 9:05 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > 2022-03-31 11:45 UTC-0400 ~ Milan Landaverde <milan@mdaverde.com> > > > Previously [1], we were using bpf_probe_prog_type which returned a > > > bool, but the new libbpf_probe_bpf_prog_type can return a negative > > > error code on failure. This change decides for bpftool to declare > > > a program type is not available on probe failure. > > > > > > [1] https://lore.kernel.org/bpf/20220202225916.3313522-3-andrii@kernel.org/ > > > > > > Signed-off-by: Milan Landaverde <milan@mdaverde.com> > > > --- > > > tools/bpf/bpftool/feature.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c > > > index c2f43a5d38e0..b2fbaa7a6b15 100644 > > > --- a/tools/bpf/bpftool/feature.c > > > +++ b/tools/bpf/bpftool/feature.c > > > @@ -564,7 +564,7 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types, > > > > > > res = probe_prog_type_ifindex(prog_type, ifindex); > > > } else { > > > - res = libbpf_probe_bpf_prog_type(prog_type, NULL); > > > + res = libbpf_probe_bpf_prog_type(prog_type, NULL) > 0; > > > } > > > > > > #ifdef USE_LIBCAP > > > > A completely unrelated question to you, Quentin. How hard is bpftool's > dependency on libcap? We've recently removed libcap from selftests, I > wonder if it would be possible to do that for bpftool as well to > reduce amount of shared libraries bpftool depends on. There's not a super-strong dependency on it. It's used in feature probing, for two things. First one is to be accurate when we check that the user has the right capabilities for probing efficiently the system. A workaround consists in checking that we run with uid=0 (root), although it's less accurate. Second thing is probing as an unprivileged user: if bpftool is run to probe as root but with the "unprivileged" keyword, libcap is used to drop the CAP_SYS_ADMIN and run the probes without it. I don't know if there's an easy alternative to libcap for that. Also I don't know how many people use this feature, but I remember that this was added because there was some demand at the time, so presumably there are users relying on this. This being said, libcap is optional for compiling bpftool, so you should be able to have it work just as well if the library is not available on the system? Basically you'd just lose the ability to probe as an unprivileged user. Do you need to remove the optional dependency completely? Quentin PS: Not directly related but since we're talking of libcap, we recently discovered that the lib is apparently changing errno when it maybe shouldn't and plays badly with batch mode: https://stackoverflow.com/questions/71608181/bpf-xdp-bpftool-batch-file-returns-error-reading-batch-file-failed-opera
On Fri, Apr 1, 2022 at 2:33 PM Quentin Monnet <quentin@isovalent.com> wrote: > > On Fri, 1 Apr 2022 at 19:42, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Fri, Apr 1, 2022 at 9:05 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > > > 2022-03-31 11:45 UTC-0400 ~ Milan Landaverde <milan@mdaverde.com> > > > > Previously [1], we were using bpf_probe_prog_type which returned a > > > > bool, but the new libbpf_probe_bpf_prog_type can return a negative > > > > error code on failure. This change decides for bpftool to declare > > > > a program type is not available on probe failure. > > > > > > > > [1] https://lore.kernel.org/bpf/20220202225916.3313522-3-andrii@kernel.org/ > > > > > > > > Signed-off-by: Milan Landaverde <milan@mdaverde.com> > > > > --- > > > > tools/bpf/bpftool/feature.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c > > > > index c2f43a5d38e0..b2fbaa7a6b15 100644 > > > > --- a/tools/bpf/bpftool/feature.c > > > > +++ b/tools/bpf/bpftool/feature.c > > > > @@ -564,7 +564,7 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types, > > > > > > > > res = probe_prog_type_ifindex(prog_type, ifindex); > > > > } else { > > > > - res = libbpf_probe_bpf_prog_type(prog_type, NULL); > > > > + res = libbpf_probe_bpf_prog_type(prog_type, NULL) > 0; > > > > } > > > > > > > > #ifdef USE_LIBCAP > > > > > > > A completely unrelated question to you, Quentin. How hard is bpftool's > > dependency on libcap? We've recently removed libcap from selftests, I > > wonder if it would be possible to do that for bpftool as well to > > reduce amount of shared libraries bpftool depends on. > > There's not a super-strong dependency on it. It's used in feature > probing, for two things. > > First one is to be accurate when we check that the user has the right > capabilities for probing efficiently the system. A workaround consists > in checking that we run with uid=0 (root), although it's less > accurate. > > Second thing is probing as an unprivileged user: if bpftool is run to > probe as root but with the "unprivileged" keyword, libcap is used to > drop the CAP_SYS_ADMIN and run the probes without it. I don't know if > there's an easy alternative to libcap for that. Also I don't know how > many people use this feature, but I remember that this was added > because there was some demand at the time, so presumably there are > users relying on this. > > This being said, libcap is optional for compiling bpftool, so you > should be able to have it work just as well if the library is not > available on the system? Basically you'd just lose the ability to > probe as an unprivileged user. Do you need to remove the optional > dependency completely? Well, see recent patches from Martin: 82cb2b30773e bpf: selftests: Remove libcap usage from test_progs b1c2768a82b9 bpf: selftests: Remove libcap usage from test_verifier 663af70aabb7 bpf: selftests: Add helpers to directly use the capget and capset syscall We completely got rid of libcap dependency and ended up with more straightforward code. So I was wondering if this is possible for bpftool. The less unnecessary library dependencies - the better. The less feature detection -- the better. That's my only line of thought here :) > > Quentin > > PS: Not directly related but since we're talking of libcap, we > recently discovered that the lib is apparently changing errno when it > maybe shouldn't and plays badly with batch mode: > https://stackoverflow.com/questions/71608181/bpf-xdp-bpftool-batch-file-returns-error-reading-batch-file-failed-opera just another argument in favor of getting rid of extra layers of dependencies
diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c index c2f43a5d38e0..b2fbaa7a6b15 100644 --- a/tools/bpf/bpftool/feature.c +++ b/tools/bpf/bpftool/feature.c @@ -564,7 +564,7 @@ probe_prog_type(enum bpf_prog_type prog_type, bool *supported_types, res = probe_prog_type_ifindex(prog_type, ifindex); } else { - res = libbpf_probe_bpf_prog_type(prog_type, NULL); + res = libbpf_probe_bpf_prog_type(prog_type, NULL) > 0; } #ifdef USE_LIBCAP
Previously [1], we were using bpf_probe_prog_type which returned a bool, but the new libbpf_probe_bpf_prog_type can return a negative error code on failure. This change decides for bpftool to declare a program type is not available on probe failure. [1] https://lore.kernel.org/bpf/20220202225916.3313522-3-andrii@kernel.org/ Signed-off-by: Milan Landaverde <milan@mdaverde.com> --- tools/bpf/bpftool/feature.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)