From: John Fastabend <john.fastabend@gmail.com>
To: ast@kernel.org, daniel@iogearbox.net, kafai@fb.com
Cc: netdev@vger.kernel.org
Subject: [bpf PATCH v5 0/4] BPF fixes for sockhash
Date: Sat, 30 Jun 2018 06:17:31 -0700 [thread overview]
Message-ID: <20180630131506.17344.7833.stgit@john-Precision-Tower-5810> (raw)
This addresses two syzbot issues that lead to identifying (by Eric and
Wei) a class of bugs where we don't correctly check for IPv4/v6
sockets and their associated state. The second issue was a locking
omission in sockhash.
The first patch addresses IPv6 socks and fixing an error where
sockhash would overwrite the prot pointer with IPv4 prot. To fix
this build similar solution to TLS ULP. Although we continue to
allow socks in all states not just ESTABLISH in this patch set
because as Martin points out there should be no issue with this
on the sockmap ULP because we don't use the ctx in this code. Once
multiple ULPs coexist we may need to revisit this. However we
can do this in *next trees.
The other issue syzbot found that the tcp_close() handler missed
locking the hash bucket lock which could result in corrupting the
sockhash bucket list if delete and close ran at the same time.
And also the smap_list_remove() routine was not working correctly
at all. This was not caught in my testing because in general my
tests (to date at least lets add some more robust selftest in
bpf-next) do things in the "expected" order, create map, add socks,
delete socks, then tear down maps. The tests we have that do the
ops out of this order where only working on single maps not multi-
maps so we never saw the issue. Thanks syzbot. The fix is to
restructure the tcp_close() lock handling. And fix the obvious
bug in smap_list_remove().
Finally, during review I noticed the release handler was omitted
from the upstream code (patch 4) due to an incorrect merge conflict
fix when I ported the code to latest bpf-next before submitting.
This would leave references to the map around if the user never
closes the map.
v3: rework patches, dropping ESTABLISH check and adding rcu
annotation along with the smap_list_remove fix
v4: missed one more case where maps was being accessed without
the sk_callback_lock, spoted by Martin as well.
v5: changed to use a specific lock for maps and reduced callback
lock so that it is only used to gaurd sk callbacks. I think
this makes the logic a bit cleaner and avoids confusion
ovoer what each lock is doing.
Also big thanks to Martin for thorough review he caught at least
one case where I missed a rcu_call().
---
John Fastabend (4):
bpf: sockmap, fix crash when ipv6 sock is added
bpf: sockmap, fix smap_list_map_remove when psock is in many maps
bpf: sockhash fix omitted bucket lock in sock_close
bpf: sockhash, add release routine
kernel/bpf/sockmap.c | 236 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 166 insertions(+), 70 deletions(-)
next reply other threads:[~2018-06-30 13:18 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-30 13:17 John Fastabend [this message]
2018-06-30 13:17 ` [bpf PATCH v5 1/4] bpf: sockmap, fix crash when ipv6 sock is added John Fastabend
2018-06-30 13:17 ` [bpf PATCH v5 2/4] bpf: sockmap, fix smap_list_map_remove when psock is in many maps John Fastabend
2018-06-30 13:17 ` [bpf PATCH v5 3/4] bpf: sockhash fix omitted bucket lock in sock_close John Fastabend
2018-06-30 13:17 ` [bpf PATCH v5 4/4] bpf: sockhash, add release routine John Fastabend
2018-06-30 23:58 ` [bpf PATCH v5 0/4] BPF fixes for sockhash Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180630131506.17344.7833.stgit@john-Precision-Tower-5810 \
--to=john.fastabend@gmail.com \
--cc=ast@kernel.org \
--cc=daniel@iogearbox.net \
--cc=kafai@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).