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 08:14:17 +0100
From:   Björn Töpel <bjorn.topel@...el.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Björn Töpel <bjorn.topel@...il.com>
Cc:     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 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.


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;                                                  \
>> +       })
> 
> [...]
>