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] [day] [month] [year] [list]

Date:   Thu, 19 Dec 2019 10:14:18 +0100
From:   Paul Chaignon <paul.chaignon@...nge.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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

Thanks everyone for the reviews and suggestions!

On Wed, Dec 18, 2019 at 10:00:44AM -0800, Alexei Starovoitov wrote:
> 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?

Sure.  I have a BPF program that matches incoming packets against
packet-filtering rules, with a NIC that steers some flows (correspond to
different tenants) to specific CPUs.  Rules are stored in a per-cpu
hashmap with a counter (counting matching packets) and an off/on switch.
To enable a rule on a new CPU, I lookup the value for that rule, switch
the on/off variable in the value corresponding to the given CPU and
update the map.  However, the counters corresponding to other CPUs for
that same rule might have been updated between the lookup and the
update.

Other BPF users have requested the same feature before on the bcc
repository [1].  They probably have different (tracing-related?) use
cases.

1 - https://github.com/iovisor/bcc/issues/1886

> 
> >   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?

You mean to update the value corresponding to the CPU on which the
userspace program is running?  BPF_F_CURRENT_CPU is a mask on the lower 
32 bits so it would clash with existing flags, but there's probably
another way to implement the same.  Not sure I see the use case though;
userspace programs can easily update the value for the current CPU with
BPF_F_CPU.

> 
> And for consistency support this flag in map_lookup_elem too?

Sure, I'll add it to the v2.

> 
> > +
> > +/* CPU mask for single-cpu updates */
> > +#define BPF_CPU_MASK	0xFFFFFFFF00000000ULL
> 
> what is the reason to expose this in uapi?

No reason; that's a mistake.

> 
> >  /* 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)
>