Re: [PATCH bpf-next 4/5] bpf: load and verify kernel module BTFs

[Andrii Nakryiko]

On Thu, Nov 5, 2020 at 10:44 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Nov 05, 2020 at 09:51:09PM -0800, Andrii Nakryiko wrote:
> > Add kernel module listener that will load/validate and unload module BTF.
> > Module BTFs gets ID generated for them, which makes it possible to iterate
> > them with existing BTF iteration API. They are given their respective module's
> > names, which will get reported through GET_OBJ_INFO API. They are also marked
> > as in-kernel BTFs for tooling to distinguish them from user-provided BTFs.
> >
> > Also, similarly to vmlinux BTF, kernel module BTFs are exposed through
> > sysfs as /sys/kernel/btf/<module-name>. This is convenient for user-space
> > tools to inspect module BTF contents and dump their types with existing tools:
> >
> > [vmuser@archvm bpf]$ ls -la /sys/kernel/btf
> > total 0
> > drwxr-xr-x  2 root root       0 Nov  4 19:46 .
> > drwxr-xr-x 13 root root       0 Nov  4 19:46 ..
> >
> > ...
> >
> > -r--r--r--  1 root root     888 Nov  4 19:46 irqbypass
> > -r--r--r--  1 root root  100225 Nov  4 19:46 kvm
> > -r--r--r--  1 root root   35401 Nov  4 19:46 kvm_intel
> > -r--r--r--  1 root root     120 Nov  4 19:46 pcspkr
> > -r--r--r--  1 root root     399 Nov  4 19:46 serio_raw
> > -r--r--r--  1 root root 4094095 Nov  4 19:46 vmlinux
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> >  Documentation/ABI/testing/sysfs-kernel-btf |   8 +
> >  include/linux/bpf.h                        |   2 +
> >  include/linux/module.h                     |   4 +
> >  kernel/bpf/btf.c                           | 193 +++++++++++++++++++++
> >  kernel/bpf/sysfs_btf.c                     |   2 +-
> >  kernel/module.c                            |  32 ++++
> >  6 files changed, 240 insertions(+), 1 deletion(-)
> >

[...]

> > +             if (IS_ENABLED(CONFIG_SYSFS)) {
> > +                     struct bin_attribute *attr;
> > +
> > +                     attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> > +                     if (!attr) {
> > +                             WARN(1, "failed to register module [%s] BTF in sysfs\n", mod->name);
>
> kzalloc() will print errors on its own, no need to do this again.  Also,
> for systems with panic-on-warn, you just crashed them, not nice :(

ah, pr_warn() is what I probably wanted here instead of WARN. I'll
just drop this, if kzalloc will log a warning anyways.

>
> > +                             goto out;
> > +                     }
> > +
> > +                     attr->attr.name = btf->name;
> > +                     attr->attr.mode = 0444;
> > +                     attr->size = btf->data_size;
> > +                     attr->private = btf;
> > +                     attr->read = btf_module_read;
> > +
> > +                     err = sysfs_create_bin_file(btf_kobj, attr);
>
> You forgot to call sysfs_bin_attr_init() to initialize your binary sysfs
> attribute.  You'll only notice if you turn lockdep on.

Good catch, fixed. Also added CONFIG_DEBUG_LOCK_ALLOC to my config.

>
>
> > +                     if (err) {
> > +                             kfree(attr);
> > +                             WARN(1, "failed to register module [%s] BTF in sysfs: %d\n",
> > +                                  mod->name, err);
>
> Again, just report the error and move on, don't crash systems.
>

Right, fixed.

> Other than these minor things, looks good to me, nice work!
>

Thanks!

> thanks,
>
> greg k-h