netdev - Re: [PATCH bpf-next 2/2] libbpf, xsk: add libbpf_smp_store_release libbpf_smp_load

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]

Date:   Wed, 3 Mar 2021 09:19:13 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Björn Töpel <bjorn.topel@...el.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>, maximmi@...dia.com,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH bpf-next 2/2] libbpf, xsk: add libbpf_smp_store_release libbpf_smp_load_acquire

On Wed, 3 Mar 2021 at 08:14, Björn Töpel <bjorn.topel@...el.com> wrote:
>
> On 2021-03-03 05:38, Andrii Nakryiko wrote:
> > On Mon, Mar 1, 2021 at 2:43 AM Björn Töpel <bjorn.topel@...il.com> wrote:
> >>
> >> From: Björn Töpel <bjorn.topel@...el.com>
> >>
> >> Now that the AF_XDP rings have load-acquire/store-release semantics,
> >> move libbpf to that as well.
> >>
> >> The library-internal libbpf_smp_{load_acquire,store_release} are only
> >> valid for 32-bit words on ARM64.
> >>
> >> Also, remove the barriers that are no longer in use.
> >>
> >> Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
> >> ---
> >>   tools/lib/bpf/libbpf_util.h | 72 +++++++++++++++++++++++++------------
> >>   tools/lib/bpf/xsk.h         | 17 +++------
> >>   2 files changed, 55 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf_util.h b/tools/lib/bpf/libbpf_util.h
> >> index 59c779c5790c..94a0d7bb6f3c 100644
> >> --- a/tools/lib/bpf/libbpf_util.h
> >> +++ b/tools/lib/bpf/libbpf_util.h
> >> @@ -5,6 +5,7 @@
> >>   #define __LIBBPF_LIBBPF_UTIL_H
> >>
> >>   #include <stdbool.h>
> >> +#include <linux/compiler.h>
> >>
> >>   #ifdef __cplusplus
> >>   extern "C" {
> >> @@ -15,29 +16,56 @@ extern "C" {
> >>    * application that uses libbpf.
> >>    */
> >>   #if defined(__i386__) || defined(__x86_64__)
> >> -# define libbpf_smp_rmb() asm volatile("" : : : "memory")
> >> -# define libbpf_smp_wmb() asm volatile("" : : : "memory")
> >> -# define libbpf_smp_mb() \
> >> -       asm volatile("lock; addl $0,-4(%%rsp)" : : : "memory", "cc")
> >> -/* Hinders stores to be observed before older loads. */
> >> -# define libbpf_smp_rwmb() asm volatile("" : : : "memory")
> >
> > So, technically, these four are part of libbpf's API, as libbpf_util.h
> > is actually installed on target hosts. Seems like xsk.h is the only
> > one that is using them, though.
> >
> > So the question is whether it's ok to remove them now?
> >
>
> I would say that. Ideally, the barriers shouldn't be visible at all,
> since they're only used as an implementation detail for the static
> inline functions.
>
>
> > And also, why wasn't this part of xsk.h in the first place?
> >
>
> I guess there was a "maybe it can be useful for more than the XDP socket
> parts of libbpf"-idea. I'll move them to xsk.h for the v2, which will
> make the migration easier.
>

Clarification! The reason for not having them in xsk.h, was that the
idea was that only the APIs allowed from the application should reside
there. IOW, libbpf_utils.h is only "implementation details". Again,
the static-inline function messes things up. Maybe moving to an
LTO-only world would be better, so we can get rid of the inlining all
together.

>
> Björn
>
>
> >> +# define libbpf_smp_store_release(p, v)                                        \
> >> +       do {                                                            \
> >> +               asm volatile("" : : : "memory");                        \
> >> +               WRITE_ONCE(*p, v);                                      \
> >> +       } while (0)
> >> +# define libbpf_smp_load_acquire(p)                                    \
> >> +       ({                                                              \
> >> +               typeof(*p) ___p1 = READ_ONCE(*p);                       \
> >> +               asm volatile("" : : : "memory");                        \
> >> +               ___p1;                                                  \
> >> +       })
> >
> > [...]
> >