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: <CAEf4BzbeUu78v63UmDCzQ5jDhBqzaX-85GHdqc1aMuiy7ZMn3w@mail.gmail.com>
Date:   Wed, 2 Dec 2020 14:43:29 -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 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?

>
> The rest of the series look good to me.

Cool, thanks.