| 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: <808092d9-e29d-05b6-1e18-994bdfc61fab@fb.com>
Date: Mon, 16 May 2022 13:03:48 -0700
From: Yonghong Song <yhs@...com>
To: Larysa Zaremba <larysa.zaremba@...el.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org,
linux-kernel@...r.kernel.org, Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>,
Quentin Monnet <quentin@...valent.com>,
Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
Alexander Lobakin <alexandr.lobakin@...el.com>
Subject: Re: [PATCH bpf-next v3] bpftool: Use sysfs vmlinux when dumping BTF
by ID
On 5/13/22 5:17 AM, Larysa Zaremba wrote:
> Currently, dumping almost all BTFs specified by id requires
> using the -B option to pass the base BTF. For kernel module
> BTFs the vmlinux BTF sysfs path should work.
This is not precise. It should be that
dumping all module BTFs specified by id requires using the -B
option. In current situation, to dump a btf associated with
a bpf program, -B option is not needed.
>
> This patch simplifies dumping by ID usage by loading
> vmlinux BTF from sysfs as base, if base BTF was not specified
> and the ID corresponds to a kernel module BTF.
>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> Reviewed-by: Alexander Lobakin <alexandr.lobakin@...el.com>
> ---
> From v2[0]:
> - instead of using vmlinux as base only after the first unsuccessful
> attempt, set base to vmlinux before loading in applicable cases, precisely
> if no base was provided by user and id belongs to a kernel module BTF.
>
> From v1[1]:
> - base BTF is assumed to be vmlinux only for kernel BTFs.
>
> [0] https://lore.kernel.org/bpf/20220505130507.130670-1-larysa.zaremba@intel.com/
> [1] https://lore.kernel.org/bpf/20220428111442.111805-1-larysa.zaremba@intel.com/
> ---
> tools/bpf/bpftool/btf.c | 65 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index a2c665beda87..0eb105c416fc 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -459,6 +459,54 @@ static int dump_btf_c(const struct btf *btf,
> return err;
> }
>
> +static const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux";
> +
> +static struct btf *get_vmlinux_btf_from_sysfs(void)
> +{
> + struct btf *base;
> +
> + base = btf__parse(sysfs_vmlinux, NULL);
> + if (libbpf_get_error(base)) {
> + p_err("failed to parse vmlinux BTF at '%s': %ld\n",
> + sysfs_vmlinux, libbpf_get_error(base));
nit: libbpf_get_error() is called twice. Can be consolidated into
one call.
> + base = NULL;
> + }
> +
> + return base;
> +}
> +
> +#define BTF_NAME_BUFF_LEN 64
> +
> +static bool btf_is_kernel_module(__u32 btf_id)
> +{
> + struct bpf_btf_info btf_info = {};
> + char btf_name[BTF_NAME_BUFF_LEN];
> + int btf_fd;
> + __u32 len;
> + int err;
> +
> + btf_fd = bpf_btf_get_fd_by_id(btf_id);
> + if (btf_fd < 0) {
> + p_err("can't get BTF object by id (%u): %s",
> + btf_id, strerror(errno));
I am not sure whether we want p_err here or not.
The function is to test whether btf is a kernel_module.
If anything wrong happens here. The original logic
follows and an error will be printed out any way.
> + return false;
> + }
> +
> + len = sizeof(btf_info);
> + btf_info.name = ptr_to_u64(btf_name);
> + btf_info.name_len = sizeof(btf_name);
> + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> + close(btf_fd);
> +
> + if (err) {
> + p_err("can't get BTF (ID %u) object info: %s",
> + btf_id, strerror(errno));
The same as above, probably we don't need p_err here.
> + return false;
> + }
> +
> + return strncmp(btf_name, "vmlinux", sizeof(btf_name)) && btf_info.kernel_btf;
This should work considering current module names but the code itself
doesn't sound right. The characters after "vmlinux" could be arbitrary.
strncmp(btf_name, "vmlinux", strlen("vmlinux") + 1) is better.
> +}
> +
> static int do_dump(int argc, char **argv)
> {
> struct btf *btf = NULL, *base = NULL;
> @@ -536,18 +584,11 @@ static int do_dump(int argc, char **argv)
> NEXT_ARG();
> } else if (is_prefix(src, "file")) {
> const char sysfs_prefix[] = "/sys/kernel/btf/";
> - const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux";
>
> if (!base_btf &&
> strncmp(*argv, sysfs_prefix, sizeof(sysfs_prefix) - 1) == 0 &&
> - strcmp(*argv, sysfs_vmlinux) != 0) {
> - base = btf__parse(sysfs_vmlinux, NULL);
> - if (libbpf_get_error(base)) {
> - p_err("failed to parse vmlinux BTF at '%s': %ld\n",
> - sysfs_vmlinux, libbpf_get_error(base));
> - base = NULL;
> - }
> - }
> + strcmp(*argv, sysfs_vmlinux))
> + base = get_vmlinux_btf_from_sysfs();
>
> btf = btf__parse_split(*argv, base ?: base_btf);
> err = libbpf_get_error(btf);
> @@ -591,6 +632,12 @@ static int do_dump(int argc, char **argv)
> }
>
> if (!btf) {
> + if (!base_btf && btf_is_kernel_module(btf_id)) {
> + p_info("Warning: valid base BTF was not specified with -B option, falling back on standard base BTF (%s)",
> + sysfs_vmlinux);
Having 'Warning' for p_info seems not appropriate. Similar to other
p_info, you can just remove 'Warning: ".
> + base_btf = get_vmlinux_btf_from_sysfs();
> + }
> +
> btf = btf__load_from_kernel_by_id_split(btf_id, base_btf);
> err = libbpf_get_error(btf);
> if (err) {
Powered by blists - more mailing lists