On Fri, May 1, 2020 at 2:56 AM Eelco Chaudron <echaudro@xxxxxxxxxx> wrote:
>
>
>
> On 30 Apr 2020, at 20:12, Andrii Nakryiko wrote:
>
> > On Thu, Apr 30, 2020 at 3:24 AM Eelco Chaudron <echaudro@xxxxxxxxxx>
> > wrote:
> >>
> >> When the probe code was failing for any reason ENOTSUP was returned,
> >> even
> >> if this was due to no having enough lock space. This patch fixes this
> >> by
> >> returning EPERM to the user application, so it can respond and
> >> increase
> >> the RLIMIT_MEMLOCK size.
> >>
> >> Signed-off-by: Eelco Chaudron <echaudro@xxxxxxxxxx>
> >> ---
> >> tools/lib/bpf/libbpf.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 8f480e29a6b0..a62388a151d4 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -3381,8 +3381,13 @@ bpf_object__probe_caps(struct bpf_object *obj)
> >>
> >> for (i = 0; i < ARRAY_SIZE(probe_fn); i++) {
> >> ret = probe_fn[i](obj);
> >> - if (ret < 0)
> >> + if (ret < 0) {
> >> pr_debug("Probe #%d failed with %d.\n", i,
> >> ret);
> >> + if (ret == -EPERM) {
> >> + pr_perm_msg(ret);
> >> + return ret;
> >
> > I think this is dangerous to do. This detection loop is not supposed
> > to return error to user if any of the features are missing. I'd feel
> > more comfortable if we split bpf_object__probe_name() into two tests:
> > one testing trivial program and another testing same program with
> > name. If the first one fails with EPERM -- then we can return error to
> > user. If anything else fails -- that's ok. Thoughts?
>
> Before sending the patch I briefly checked the existing probes and did
> not see any other code path that could lead to EPERM. But you are right
> that this might not be the case for previous kernels. So you suggest
> something like this?
It both previous as well as future kernel version. We can never be
100% sure. While the idea of probe_caps() is to detect optional
features.
>
> diff --git a/src/libbpf.c b/src/libbpf.c
> index ff91742..fd5fdee 100644
> --- a/src/libbpf.c
> +++ b/src/libbpf.c
> @@ -3130,7 +3130,7 @@ int bpf_map__resize(struct bpf_map *map, __u32
> max_entries)
> }
>
> static int
> -bpf_object__probe_name(struct bpf_object *obj)
> +bpf_object__probe_loading(struct bpf_object *obj)
> {
> struct bpf_load_program_attr attr;
> char *cp, errmsg[STRERR_BUFSIZE];
> @@ -3157,8 +3157,26 @@ bpf_object__probe_name(struct bpf_object *obj)
> }
> close(ret);
>
> - /* now try the same program, but with the name */
> + return 0;
> +}
>
> +static int
> +bpf_object__probe_name(struct bpf_object *obj)
> +{
> + struct bpf_load_program_attr attr;
> + struct bpf_insn insns[] = {
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + };
> + int ret;
> +
> + /* make sure loading with name works */
> +
> + memset(&attr, 0, sizeof(attr));
> + attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> + attr.insns = insns;
> + attr.insns_cnt = ARRAY_SIZE(insns);
> + attr.license = "GPL";
> attr.name = "test";
> ret = bpf_load_program_xattr(&attr, NULL, 0);
> if (ret >= 0) {
> @@ -3328,6 +3346,11 @@ bpf_object__probe_caps(struct bpf_object *obj)
> };
> int i, ret;
>
> + if (bpf_object__probe_loading(obj) == -EPERM) {
> + pr_perm_msg(-EPERM);
> + return -EPERM;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(probe_fn); i++) {
> ret = probe_fn[i](obj);
> if (ret < 0)
>
> Let me know, and I sent out a v2.
Yes, that's the split I had in mind, but I'd move
bpf_object__probe_loading() call directly into bpf_object__load() to
be the first thing to check. probe_caps() should still be non-failing
if any feature is missing. Does it make sense?
>
> Cheers,
>
> Eelco
>
