netdev - Re: [PATCH v4 bpf-next 10/14] bpf: allow to specify kernel module BTFs when attaching BPF programs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 2 Dec 2020 15:14:39 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v4 bpf-next 10/14] bpf: allow to specify kernel module
 BTFs when attaching BPF programs

On Wed, Dec 2, 2020 at 3:12 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Dec 2, 2020 at 2:43 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Wed, Dec 2, 2020 at 12:58 PM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Tue, Dec 01, 2020 at 04:16:12PM -0800, Andrii Nakryiko wrote:
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index c3458ec1f30a..60b95b51ccb8 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -558,6 +558,7 @@ union bpf_attr {
> > > >               __u32           line_info_cnt;  /* number of bpf_line_info records */
> > > >               __u32           attach_btf_id;  /* in-kernel BTF type id to attach to */
> > > >               __u32           attach_prog_fd; /* 0 to attach to vmlinux */
> > > > +             __u32           attach_btf_obj_id; /* vmlinux/module BTF object ID for BTF type */
> > >
> > > I think the uapi should use attach_btf_obj_fd here.
> > > Everywhere else uapi is using FDs to point to maps, progs, BTFs of progs.
> > > BTF of a module isn't different from BTF of a program.
> > > Looking at libbpf implementation... it has the FD of a module anyway,
> > > since it needs to fetch it to search for the function btf_id in there.
> > > So there won't be any inconvenience for libbpf to pass FD in here.
> > > From the uapi perspective attach_btf_obj_fd will remove potential
> > > race condition. It's very unlikely race, of course.
> >
> > Yes, I actually contemplated that, but my preference went the ID way,
> > because it made libbpf implementation simpler and there was a nice
> > duality of using ID for types and BTF instances themselves.
> >
> > The problem with FD is that when I load all module BTF objects, I open
> > their FD one at a time, and close it as soon as I read BTF raw data
> > back. If I don't do that on systems with many modules, I'll be keeping
> > potentially hundreds of FDs open, so I figured I don't want to do
> > that.
> >
> > But I do see the FD instead of ID consistency as well, so I can go
> > with a simple and inefficient implementation of separate FD for each
> > BTF object for now, and if someone complains, we can teach libbpf to
> > lazily open FDs of module BTFs that are actually used (later, it will
> > complicate code unnecessarily). Not really worried about racing with
> > kernel modules being unloaded.
> >
> > Also, if we use FD, we might not need a new attach_bpf_obj_id field at
> > all, we can re-use attach_prog_fd field (put it in union and have
> > attach_prog_fd/attach_btf_fd). On the kernel side, it would be easy to
> > check whether provided FD is for bpf_prog or btf. What do you think?
> > Too mysterious? Or good?
>
> You mean like:
> union {
>          __u32           attach_prog_fd; /* valid prog_fd to attach to
> bpf prog */
>          __u32           attach_btf_obj_fd; /* or  valid module BTF
> object fd or zero to attach to vmlinux */
> };

like this with union, an aliased field name with a meaningful name

> or don't introduce a new field name at all?
> Sure. I'm fine with both. I think it's a good idea.

ok, will do this then