bpf, sockmap: fix sock_map_ctx_update_elem race with exist/noexist
authorDaniel Borkmann <daniel@iogearbox.net>
Thu, 16 Aug 2018 19:49:10 +0000 (21:49 +0200)
committerAlexei Starovoitov <ast@kernel.org>
Thu, 16 Aug 2018 21:58:08 +0000 (14:58 -0700)
commit585f5a6252ee43ec8feeee07387e3fcc7e8bb292
tree336d9a13409cccdc8d2c8f843bae5f62f4a9334b
parent166ab6f0a0702fdd4d865ad5090bf3094ed83428
bpf, sockmap: fix sock_map_ctx_update_elem race with exist/noexist

The current code in sock_map_ctx_update_elem() allows for BPF_EXIST
and BPF_NOEXIST map update flags. While on array-like maps this approach
is rather uncommon, e.g. bpf_fd_array_map_update_elem() and others
enforce map update flags to be BPF_ANY such that xchg() can be used
directly, the current implementation in sock map does not guarantee
that such operation with BPF_EXIST / BPF_NOEXIST is atomic.

The initial test does a READ_ONCE(stab->sock_map[i]) to fetch the
socket from the slot which is then tested for NULL / non-NULL. However
later after __sock_map_ctx_update_elem(), the actual update is done
through osock = xchg(&stab->sock_map[i], sock). Problem is that in
the meantime a different CPU could have updated / deleted a socket
on that specific slot and thus flag contraints won't hold anymore.

I've been thinking whether best would be to just break UAPI and do
an enforcement of BPF_ANY to check if someone actually complains,
however trouble is that already in BPF kselftest we use BPF_NOEXIST
for the map update, and therefore it might have been copied into
applications already. The fix to keep the current behavior intact
would be to add a map lock similar to the sock hash bucket lock only
for covering the whole map.

Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
kernel/bpf/sockmap.c