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 PHC | |
Open Source and information security mailing list archives
| ||
|
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