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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAADnVQ+6QAGSgaXZu7g75suOcG+KEanoJNb6DP-jFKoHZKKxiA@mail.gmail.com>
Date:   Wed, 2 Dec 2020 15:12:25 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...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 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 */
};
or don't introduce a new field name at all?
Sure. I'm fine with both. I think it's a good idea.