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  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]
Date:   Mon, 14 Sep 2020 16:10:11 -0700
From:   Andrii Nakryiko <>
To:     Toke Høiland-Jørgensen <>
Cc:     Alexei Starovoitov <>,
        Daniel Borkmann <>,
        Martin KaFai Lau <>,
        Song Liu <>, Yonghong Song <>,
        Andrii Nakryiko <>,
        John Fastabend <>,
        Jiri Olsa <>,
        Eelco Chaudron <>,
        KP Singh <>,
        Networking <>, bpf <>
Subject: Re: [PATCH RESEND bpf-next v3 4/9] bpf: support attaching freplace
 programs to multiple attach points

On Mon, Sep 14, 2020 at 9:08 AM Toke Høiland-Jørgensen <> wrote:
> Andrii Nakryiko <> writes:
> > On Fri, Sep 11, 2020 at 3:01 AM Toke Høiland-Jørgensen <> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <>
> >>
> >> This enables support for attaching freplace programs to multiple attach
> >> points. It does this by amending UAPI for bpf_raw_tracepoint_open with a
> >> target prog fd and btf ID pair that can be used to supply the new
> >> attachment point. The target must be compatible with the target that was
> >> supplied at program load time.
> >>
> >> The implementation reuses the checks that were factored out of
> >> check_attach_btf_id() to ensure compatibility between the BTF types of the
> >> old and new attachment. If these match, a new bpf_tracing_link will be
> >> created for the new attach target, allowing multiple attachments to
> >> co-exist simultaneously.
> >>
> >> The code could theoretically support multiple-attach of other types of
> >> tracing programs as well, but since I don't have a use case for any of
> >> those, the bpf_tracing_prog_attach() function will reject new targets for
> >> anything other than PROG_TYPE_EXT programs.
> >>
> >> Signed-off-by: Toke Høiland-Jørgensen <>
> >> ---
> >
> > It feels like using a semi-constructed bpf_tracing_link inside
> > prog->aux->tgt_link is just an unnecessary complication, after reading
> > this and previous patches. Seems more straightforward and simpler to
> > store tgt_attach_type/tgt_prog_type (permanently) and
> > tgt_prog/tgt_trampoline (until first attachment) in prog->aux and then
> > properly create bpf_link on attach.
> I updated v4 with your comments, but kept the link in prog->aux; the
> reason being that having a container for the two pointers makes it
> possible to atomically swap it out with xchg() as you suggested
> previously. Could you please take a look at v4? If you still think it's
> better to just keep two separate pointers (and add a lock) in prog->aux,
> I can change it to that. But I'd rather avoid the lock if possible...

I took a very quick look at this specific bit, planning to do another
pass tomorrow.

What's the problem with adding a mutex to bpf_prog_aux? In your case,
now you introduced (unlikely, but still) extra state transition for
tgt_link from non-NULL to NULL and then back to non-NULL? And why?
Just to use atomic xchg, while using atomic operation is not an
absolute necessity because it's not a performance-critical path at
all. We are not optimizing for millions of freplace attachments a
second, right? On the other hand, having a mutex there won't require
restoration logic, it will be dead simple, obvious and
straightforward. So yeah, I still think mutex is better there.

BTW, check Stanislav's latest patch set. He's adding used_maps_mutex
to bpf_prog_aux with no problems at all. It seems to me that we might
want to generalize that used_maps_mutex to be just bpf_prog_aux's
mutex ('prog_aux_mutex' or whatever we'd call it) and use it for such
kinds of low-frequency bpf_prog metadata manipulations/checks.


> -Toke

Powered by blists - more mailing lists