linux-kernel - Re: [PATCH bpf-next] libbpf: Add bpf_object_

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]

Message-ID: <CAEf4BzayCLQxmzgWkAzU-vzD9K+iDvBHkLYSx8w=da-o9dW75w@mail.gmail.com>
Date:   Wed, 23 Aug 2023 14:16:50 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Xu <dxu@...uu.xyz>
Cc:     ast@...nel.org, andrii@...nel.org, daniel@...earbox.net,
        martin.lau@...ux.dev, song@...nel.org, yonghong.song@...ux.dev,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next] libbpf: Add bpf_object__unpin()

On Wed, Aug 23, 2023 at 11:43 AM Daniel Xu <dxu@...uu.xyz> wrote:
>
> On Wed, Aug 23, 2023 at 10:19:10AM -0700, Andrii Nakryiko wrote:
> > On Tue, Aug 22, 2023 at 10:44 PM Daniel Xu <dxu@...uu.xyz> wrote:
> > >
> > > For bpf_object__pin_programs() there is bpf_object__unpin_programs().
> > > Likewise bpf_object__unpin_maps() for bpf_object__pin_maps().
> > >
> > > But no bpf_object__unpin() for bpf_object__pin(). Adding the former adds
> > > symmetry to the API.
> > >
> > > It's also convenient for cleanup in application code. It's an API I
> > > would've used if it was available for a repro I was writing earlier.
> > >
> > > Signed-off-by: Daniel Xu <dxu@...uu.xyz>
> > > ---
> > >  tools/lib/bpf/libbpf.c   | 15 +++++++++++++++
> > >  tools/lib/bpf/libbpf.h   |  1 +
> > >  tools/lib/bpf/libbpf.map |  1 +
> > >  3 files changed, 17 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 4c3967d94b6d..96ff1aa4bf6a 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -8376,6 +8376,21 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
> > >         return 0;
> > >  }
> > >
> > > +int bpf_object__unpin(struct bpf_object *obj, const char *path)
> > > +{
> > > +       int err;
> > > +
> > > +       err = bpf_object__unpin_programs(obj, path);
> > > +       if (err)
> > > +               return libbpf_err(err);
> > > +
> > > +       err = bpf_object__unpin_maps(obj, path);
> > > +       if (err)
> > > +               return libbpf_err(err);
> > > +
> > > +       return 0;
> > > +}
> > > +
> >
> > pin APIs predate me, and I barely ever use them, but I wonder if
> > people feel fine with the fact that if any single unpin fails, all the
> > other programs/maps will not be unpinned? I also wonder if the best
> > effort unpinning of everything (while propagating first/last error) is
> > more practical? Looking at bpf_object__pin_programs, we try unpin
> > everything, even if some unpins fail.
> >
> > Any thoughts or preferences?
>
> Yeah, I noticed bpf_object__pin_programs() tries to simulate some
> transactionality. However, bpf_object__unpin_programs() and
> bpf_object__unpin_maps() both do not try rollbacks and have already been
> exposed as public API. So I thought it would be best to stay consistent.

yep, makes sense. I guess if I were to rely heavily on
pinning/unpinning, I always have an option to pin/unpin individually.
Ok, please address the other feedback and resubmit.

>
> I also figured it's unlikely only a single unpin() fails. For pin(), you
> could have name collisions. But not for unpin(). I suppose the main
> error case is if some 3rd party (or yourself) comes in and messes with
> your objects in bpffs.
>
> In general, though, there are other places where transactionality would
> be a nice property. For example, if I have a TC prog that I want to
> attach to, say, _all_ ethernet interfaces, I have to be careful about
> rollbacks in the event of failure on a single iface.
>
> It would be really nice if the kernel  had a general way to provide
> atomicity w.r.t. multiple operations. But I suppose that's a hard
> problem.
>
> [...]
>
> Thanks,
> Daniel