netdev - Re: [PATCH bpf-next 1/3] bpf: add bpf queue map

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]

Message-ID: <20180810030901.dgc2sbi4zcryvx35@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 9 Aug 2018 20:09:03 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Mauricio Vasquez <mauricio.vasquez@...ito.it>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 1/3] bpf: add bpf queue map

On Thu, Aug 09, 2018 at 06:41:54PM -0500, Mauricio Vasquez wrote:
> 
> 
> On 08/09/2018 11:23 AM, Alexei Starovoitov wrote:
> > On Thu, Aug 09, 2018 at 09:51:49AM -0500, Mauricio Vasquez wrote:
> > > > Agree that existing ops are not the right alias, but deferring to user
> > > > space as inline function also doesn't really seem like a good fit, imho,
> > > > so I'd prefer rather to have something native. (Aside from that, the
> > > > above inline bpf_pop() would also race between CPUs.)
> > > I think we should have push/pop/peek syscalls as well, having a bpf_pop()
> > > that is race prone would create problems. Users expected maps operations to
> > > be safe, so having one that is not will confuse them.
> > agree the races are not acceptable.
> > How about a mixed solution:
> > - introduce bpf_push/pop/peak helpers that programs will use, so
> >    they don't need to pass useless key=NULL
> > - introduce map->ops->lookup_and_delete and map->ops->lookup_or_init
> >    that prog-side helpers can use and syscall has 1-1 mapping for
> I think if is a fair solution.
> > Native lookup_or_init() helper for programs and syscall is badly missing.
> > Most of the bcc scripts use it and bcc has a racy workaround.
> > Similarly lookup_and_delete() syscall is 1-1 to pop() for stack/queue
> > and useful for regular hash maps.
> > 
> > At the end for stack/queue map the programs will use:
> > int bpf_push(map, value);
> 
> Also flags should be passed here.

Good point about flags. Indeed flags should be there for
extensibility, but I cannot think of use case for them at the moment.
hash's exist/noexist don't apply here.
Would be good to have at least one bit in use from the start.

> 
> > value_or_null = bpf_pop(map); // guaranteed non-racy for multi-cpu
> > value_or_null = bpf_peak(map); // racy if 2+ cpus doing it
> Is there any reason for it to be racy?

because two cpus will be looking at the same element?
whethere implementation is array based or link list with rcu
the race is still there.