| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzYwqwdPK17vpiUGLBVZpcWshCezbO_jy8kj2mEnxKW0YA@mail.gmail.com>
Date: Wed, 24 Jan 2024 09:40:19 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Alan Maguire <alan.maguire@...cle.com>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] libbpf: Add some details for BTF parsing failures
On Tue, Jan 23, 2024 at 8:37 PM Ian Rogers <irogers@...gle.com> wrote:
>
> On Tue, Jan 23, 2024 at 8:25 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Tue, Jan 23, 2024 at 12:44 PM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > As CONFIG_DEBUG_INFO_BTF is default off the existing "failed to find
> > > valid kernel BTF" message makes diagnosing the kernel build issue some
> > > what cryptic. Add a little more detail with the hope of helping users.
> > >
> > > Before:
> > > ```
> > > libbpf: failed to find valid kernel BTF
> > > libbpf: Error loading vmlinux BTF: -3
> > > libbpf: failed to load object 'lock_contention_bpf'
> > > libbpf: failed to load BPF skeleton 'lock_contention_bpf': -3
> > > ```
> > >
> > > After no access /sys/kernel/btf/vmlinux:
> > > ```
> > > libbpf: Unable to access canonical vmlinux BTF from /sys/kernel/btf/vmlinux
> > > libbpf: Error loading vmlinux BTF: -3
> > > libbpf: failed to load object 'lock_contention_bpf'
> > > libbpf: failed to load BPF skeleton 'lock_contention_bpf': -3
> > > ```
> > >
> > > After no BTF /sys/kernel/btf/vmlinux:
> > > ```
> > > libbpf: Failed to load vmlinux BTF from /sys/kernel/btf/vmlinux, was CONFIG_DEBUG_INFO_BTF enabled?
> > > libbpf: Error loading vmlinux BTF: -3
> > > libbpf: failed to load object 'lock_contention_bpf'
> > > libbpf: failed to load BPF skeleton 'lock_contention_bpf': -3
> > > ```
> > >
> > > Closes: https://lore.kernel.org/bpf/CAP-5=fU+DN_+Y=Y4gtELUsJxKNDDCOvJzPHvjUVaUoeFAzNnig@mail.gmail.com/
> > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > >
> > > ---
> > > v2. Try to address review comments from Andrii Nakryiko.
> > > ---
> > > tools/lib/bpf/btf.c | 49 ++++++++++++++++++++++++++++++++-------------
> > > 1 file changed, 35 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > > index ee95fd379d4d..d8a05dda0836 100644
> > > --- a/tools/lib/bpf/btf.c
> > > +++ b/tools/lib/bpf/btf.c
> > > @@ -4920,16 +4920,25 @@ static int btf_dedup_remap_types(struct btf_dedup *d)
> > > return 0;
> > > }
> > >
> > > +static struct btf *btf__load_vmlinux_btf_path(const char *path)
> >
> > I don't think we need this helper, you literally call btf__parse() and
> > pr_debug(), that's all
> >
> > > +{
> > > + struct btf *btf;
> > > + int err;
> > > +
> > > + btf = btf__parse(path, NULL);
> > > + err = libbpf_get_error(btf);
> >
> > we should stop using libbpf_get_error, in libbpf v1.0+ it's best to do just
> >
> > btf = btf__parse(path, NULL);
> > if (!btf) {
> > err = -errno;
> > pr_debug(...);
> > return NULL;
> > }
> >
> > > + pr_debug("loading kernel BTF '%s': %d\n", path, err);
> > > + return err ? NULL : btf;
> > > +}
> > > +
> > > /*
> > > * Probe few well-known locations for vmlinux kernel image and try to load BTF
> > > * data out of it to use for target BTF.
> > > */
> > > struct btf *btf__load_vmlinux_btf(void)
> > > {
> > > + /* fall back locations, trying to find vmlinux on disk */
> > > const char *locations[] = {
> > > - /* try canonical vmlinux BTF through sysfs first */
> > > - "/sys/kernel/btf/vmlinux",
> > > - /* fall back to trying to find vmlinux on disk otherwise */
> > > "/boot/vmlinux-%1$s",
> > > "/lib/modules/%1$s/vmlinux-%1$s",
> > > "/lib/modules/%1$s/build/vmlinux",
> > > @@ -4938,29 +4947,41 @@ struct btf *btf__load_vmlinux_btf(void)
> > > "/usr/lib/debug/boot/vmlinux-%1$s.debug",
> > > "/usr/lib/debug/lib/modules/%1$s/vmlinux",
> > > };
> > > - char path[PATH_MAX + 1];
> > > + const char *location;
> > > struct utsname buf;
> > > struct btf *btf;
> > > - int i, err;
> > > + int i;
> > >
> > > - uname(&buf);
> > > + /* try canonical vmlinux BTF through sysfs first */
> > > + location = "/sys/kernel/btf/vmlinux";
> > > + if (faccessat(AT_FDCWD, location, R_OK, AT_EACCESS) == 0) {
> > > + btf = btf__load_vmlinux_btf_path(location);
> > > + if (btf)
> > > + return btf;
> > > +
> > > + pr_warn("Failed to load vmlinux BTF from %s, was CONFIG_DEBUG_INFO_BTF enabled?\n",
> > > + location);
> >
> > Mentioning CONFIG_DEBUG_INFO_BTF seems inappropriate here,
> > /sys/kernel/btf/vmlinux exists, we just failed to parse its data,
> > right? So it's not about CONFIG_DEBUG_INFO_BTF, we just don't support
> > something in BTF data. Just pr_warn("Failed to load vmlinux BTF from
> > %s: %d", location, err); should be good
>
> I think that assumes a lot about a user, they understand what BTF
> means, they know it is controlled by a kernel config option, and that
> the config option needs to be overridden (as it is defaulted off) for
> BTF to work. Given this escaped Raspberry Pi OS the potential for this
> mistake seems high - hence wanting to highlight the config option.
But there is nothing wrong with CONFIG_DEBUG_INFO_BTF, it is enabled,
and hence there is /sys/kernel/btf/vmlinux on the system. With
CONFIG_DEBUG_INFO_BTF suggestion you'll just lead users astray. What
am I missing?
>
> > > + } else
> > > + pr_warn("Unable to access canonical vmlinux BTF from %s\n", location);
> >
> > here the question of CONFIG_DEBUG_INFO_BTF is more appropriate, if
> > /sys/kernel/btf/vmlinux (on modern enough kernels) is missing, then
> > CONFIG_DEBUG_INFO_BTF is missing, probably. But I'd emit this only
> > after trying all the fallback paths and not finding anything.
> >
> > also stylistical nit: if one side of if has {}, the other has to have
> > {} as well, even if it's just one line
> >
> > >
> > > + uname(&buf);
> > > for (i = 0; i < ARRAY_SIZE(locations); i++) {
> > > - snprintf(path, PATH_MAX, locations[i], buf.release);
> > > + char path[PATH_MAX + 1];
> > > +
> > > + snprintf(path, sizeof(path), locations[i], buf.release);
> > >
> > > + btf = btf__load_vmlinux_btf_path(path);
> > > if (faccessat(AT_FDCWD, path, R_OK, AT_EACCESS))
> > > continue;
> > >
> > > - btf = btf__parse(path, NULL);
> > > - err = libbpf_get_error(btf);
> > > - pr_debug("loading kernel BTF '%s': %d\n", path, err);
> > > - if (err)
> > > - continue;
> > > + btf = btf__load_vmlinux_btf_path(location);
> > > + if (btf)
> > > + return btf;
> > >
> > > - return btf;
> > > + pr_warn("Failed to load vmlinux BTF from %s, was CONFIG_DEBUG_INFO_BTF enabled?\n",
> >
> > we should do better here as well. We should distinguish between "there
> > is vmlinux image, but it has no BTF" vs "there is no vmlinux image" vs
> > "vmlinux image is there, there is BTF, but we can't parse it". See
> > btf__parse(). We return -ENODATA if ELF doesn't have BTF, that's the
> > first situation. We can probably use faccessat() check for second
> > situation. Everything else can be reported as pr_debug() with location
> > (but still no CONFIG_DEBUG_INFO_BTF, it's meaningless for fallback BTF
> > locations)
> >
> > > + path);
> > > }
> > >
> > > - pr_warn("failed to find valid kernel BTF\n");
> >
> > and then here we can probably warn that we failed to find any kernel
> > BTF, and suggest CONFIG_DEBUG_INFO_BTF
>
> Andrii, you've basically written this patch, can I pass this over to you?
I think it would be great if you can finish thi, thanks.
>
> Thanks,
> Ian
>
> > > return libbpf_err_ptr(-ESRCH);
> > > }
> > >
> > > --
> > > 2.43.0.429.g432eaa2c6b-goog
> > >
Powered by blists - more mailing lists