netdev - Re: [PATCH bpf-next 2/3] libbpf: add bpf

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: <CAEf4BzakHPjOHcyf=idh+kMUVhk0jr=Zqd2px8vfxU5N1MV3Rg@mail.gmail.com>
Date:   Mon, 2 Mar 2020 10:09:59 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Andrii Nakryiko <andriin@...com>, 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 bpf-next 2/3] libbpf: add bpf_link pinning/unpinning

On Mon, Mar 2, 2020 at 2:17 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Andrii Nakryiko <andriin@...com> writes:
>
> > With bpf_link abstraction supported by kernel explicitly, add
> > pinning/unpinning API for links. Also allow to create (open) bpf_link from BPF
> > FS file.
> >
> > This API allows to have an "ephemeral" FD-based BPF links (like raw tracepoint
> > or fexit/freplace attachments) surviving user process exit, by pinning them in
> > a BPF FS, which is an important use case for long-running BPF programs.
> >
> > As part of this, expose underlying FD for bpf_link. While legacy bpf_link's
> > might not have a FD associated with them (which will be expressed as
> > a bpf_link with fd=-1), kernel's abstraction is based around FD-based usage,
> > so match it closely. This, subsequently, allows to have a generic
> > pinning/unpinning API for generalized bpf_link. For some types of bpf_links
> > kernel might not support pinning, in which case bpf_link__pin() will return
> > error.
> >
> > With FD being part of generic bpf_link, also get rid of bpf_link_fd in favor
> > of using vanialla bpf_link.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> >  tools/lib/bpf/libbpf.c   | 131 +++++++++++++++++++++++++++++++--------
> >  tools/lib/bpf/libbpf.h   |   5 ++
> >  tools/lib/bpf/libbpf.map |   5 ++
> >  3 files changed, 114 insertions(+), 27 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 996162801f7a..f8c4042e5855 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -6931,6 +6931,8 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> >  struct bpf_link {
> >       int (*detach)(struct bpf_link *link);
> >       int (*destroy)(struct bpf_link *link);
> > +     char *pin_path;         /* NULL, if not pinned */
> > +     int fd;                 /* hook FD, -1 if not applicable */
> >       bool disconnected;
> >  };
> >
> > @@ -6960,26 +6962,109 @@ int bpf_link__destroy(struct bpf_link *link)
> >               err = link->detach(link);
> >       if (link->destroy)
> >               link->destroy(link);
> > +     if (link->pin_path)
> > +             free(link->pin_path);
>
> This will still detach the link even if it's pinned, won't it? What's

No, this will just free pin_path string memory.

> the expectation, that the calling application just won't call
> bpf_link__destroy() if it pins the link? But then it will leak memory?
> Or is it just that __destroy() will close the fd, but if it's pinned the
> kernel won't actually detach anything? In that case, it seems like the
> function name becomes somewhat misleading?

Yes, the latter, it will close its own FD, but if someone else has
open other FD against the same bpf_link (due to pinning or if you
shared FD with child process, etc), then kernel will keep it.
bpf_link__destroy() is more of a "sever the link my process has" or
"destroy my local link". Maybe not ideal name, but should be close
enough, I think.

>
> -Toke
>