netdev - Re: [PATCH bpf-next] xsk: update rings for load-acquire/store-release semantics

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:   Tue, 21 Jan 2020 12:50:23 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        bpf <bpf@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        mark.rutland@....com, will@...nel.org
Subject: Re: [PATCH bpf-next] xsk: update rings for load-acquire/store-release semantics

On Tue, 21 Jan 2020 at 00:51, Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 1/20/20 10:21 AM, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@...el.com>
> >
> > Currently, the AF_XDP rings uses fences for the kernel-side
> > produce/consume functions. By updating rings for
> > load-acquire/store-release semantics, the full barrier (smp_mb()) on
> > the consumer side can be replaced.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
>
> If I'm not missing something from the ring update scheme, don't you also need
> to adapt to STORE.rel ->producer with matching barrier in tools/lib/bpf/xsk.h ?
>

Daniel/John,

Hmm, I was under the impression that *wasn't* the case. Quoting
memory-barriers.txt:

--8<--
When dealing with CPU-CPU interactions, certain types of memory
barrier should always be paired.  A lack of appropriate pairing is
almost certainly an error.

General barriers pair with each other, though they also pair with most
other types of barriers, albeit without multicopy atomicity.  An
acquire barrier pairs with a release barrier, but both may also pair
with other barriers, including of course general barriers.  A write
barrier pairs with a data dependency barrier, a control dependency, an
acquire barrier, a release barrier, a read barrier, or a general
barrier.  Similarly a read barrier, control dependency, or a data
dependency barrier pairs with a write barrier, an acquire barrier, a
release barrier, or a general barrier:
-->8--

(The usual "I'm not an expert, just quoting memory-barriers.txt"
disclaimer applies...)

In libbpf, we already have a "weaker" barrier (libbpf_smp_rwmb()). See
commit 2c5935f1b2b6 ("libbpf: optimize barrier for XDP socket rings")
for more information.

I agree that at some point ld.acq/st.rel barriers should be adopted in
libbpf at some point, but not that it's broken now. Then again,
reading Will's (Cc'd) perf-ring comment here [1], makes me a bit
uneasy even though he says "theoretical". ;-)

> Btw, alternative model could also be 09d62154f613 ("tools, perf: add and use
> optimized ring_buffer_{read_head, write_tail} helpers") for the kernel side
> in order to get rid of the smp_mb() on x86.
>

Interesting! I'll have a look!

[1] https://lore.kernel.org/lkml/20180523164234.GJ2983@arm.com/