netdev - Re: [PATCH bpf-next 1/3] bpf: Single-cpu updates for per-cpu maps

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, 18 Dec 2019 10:00:44 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Paul Chaignon <paul.chaignon@...nge.com>
Cc:     bpf@...r.kernel.org, paul.chaignon@...il.com,
        netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>
Subject: Re: [PATCH bpf-next 1/3] bpf: Single-cpu updates for per-cpu maps

On Wed, Dec 18, 2019 at 03:23:04PM +0100, Paul Chaignon wrote:
> Currently, userspace programs have to update the values of all CPUs at
> once when updating per-cpu maps.  This limitation prevents the update of
> a single CPU's value without the risk of missing concurrent updates on
> other CPU's values.
> 
> This patch allows userspace to update the value of a specific CPU in
> per-cpu maps.  The CPU whose value should be updated is encoded in the
> 32 upper-bits of the flags argument, as follows.  The new BPF_CPU flag
> can be combined with existing flags.

In general makes sense. Could you elaborate more on concrete issue?

>   bpf_map_update_elem(..., cpuid << 32 | BPF_CPU)
> 
> Signed-off-by: Paul Chaignon <paul.chaignon@...nge.com>
> ---
>  include/uapi/linux/bpf.h       |  4 +++
>  kernel/bpf/arraymap.c          | 19 ++++++++-----
>  kernel/bpf/hashtab.c           | 49 ++++++++++++++++++++--------------
>  kernel/bpf/local_storage.c     | 16 +++++++----
>  kernel/bpf/syscall.c           | 17 +++++++++---
>  tools/include/uapi/linux/bpf.h |  4 +++
>  6 files changed, 74 insertions(+), 35 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index dbbcf0b02970..2efb17d2c77a 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -316,6 +316,10 @@ enum bpf_attach_type {
>  #define BPF_NOEXIST	1 /* create new element if it didn't exist */
>  #define BPF_EXIST	2 /* update existing element */
>  #define BPF_F_LOCK	4 /* spin_lock-ed map_lookup/map_update */
> +#define BPF_CPU		8 /* single-cpu update for per-cpu maps */

BPF_F_CPU would be more consistent with the rest of flags.

Can BPF_F_CURRENT_CPU be supported as well?

And for consistency support this flag in map_lookup_elem too?

> +
> +/* CPU mask for single-cpu updates */
> +#define BPF_CPU_MASK	0xFFFFFFFF00000000ULL

what is the reason to expose this in uapi?

>  /* flags for BPF_MAP_CREATE command */
>  #define BPF_F_NO_PREALLOC	(1U << 0)
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index f0d19bbb9211..a96e94696819 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -302,7 +302,8 @@ static int array_map_update_elem(struct bpf_map *map, void *key, void *value,
>  	u32 index = *(u32 *)key;
>  	char *val;
>  
> -	if (unlikely((map_flags & ~BPF_F_LOCK) > BPF_EXIST))
> +	if (unlikely((map_flags & ~BPF_CPU_MASK & ~BPF_F_LOCK &
> +				  ~BPF_CPU) > BPF_EXIST))

that reads odd.
More traditional would be ~ (A | B | C)