All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Fastabend <john.fastabend@gmail.com>
To: ast@kernel.org, daniel@iogearbox.net
Cc: netdev@vger.kernel.org
Subject: [bpf PATCH v2 0/6] BPF fixes for sockhash
Date: Thu, 14 Jun 2018 09:44:41 -0700	[thread overview]
Message-ID: <20180614164148.24994.65250.stgit@john-Precision-Tower-5810> (raw)

This addresses two syzbot issues that lead to identifing (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
error in sockhash.

The first 2 patches address handling IPv4 correctly and then ensuring
that only sockets in ESTABLISHED state can be added. There is then a
follow up fix (patch4) to fix the other issue Eric noted, namely that
we depend on sockets to call tcp_close to remove them from the map.
However, we missed that a socket can transition through
tcp_disconnect() and never call tcp_close() missing our hook. To
resolve this implement the unhash hook which is also called from the
tcp_disconnect() flow.

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. To
fix this we had to restructure the tcp_close() lock handling. This is
done in patch 3.

Finally, during review I noticed the release handler was ommitted
from the upstream code (patch 5) due to an incorrect merge conflict
fix when I ported the code to latest bpf-next before submitting. And
then patch 6 fixes up selftests for the above.

The tcp_disconnect() catch also appears to be missing in kTLS so
a follow up patch will need to address that as well.

v2: Added sock lock to update paths in patch2. Martin noticed this
during review. I was planning to do this in a follow up patch but
I agree its a bit odd to not do it upfront so incorporated into
'bpf: sockmap only allow ESTABLISHED sock state'. In bpf-next we
may consider also taking sock lock on delete/map_free and which
point we could drop some usages of sk_callback_lock but need to
think a bit on the trade-offs of this.

---

John Fastabend (6):
      bpf: sockmap, fix crash when ipv6 sock is added
      bpf: sockmap only allow ESTABLISHED sock state
      bpf: sockhash fix omitted bucket lock in sock_close
      bpf: sockmap, tcp_disconnect to listen transition
      bpf: sockhash, add release routine
      bpf: selftest remove attempts to add LISTEN sockets to sockmap


 0 files changed

             reply	other threads:[~2018-06-14 16:45 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-14 16:44 John Fastabend [this message]
2018-06-14 16:44 ` [bpf PATCH v2 1/6] bpf: sockmap, fix crash when ipv6 sock is added John Fastabend
2018-06-14 23:53   ` Martin KaFai Lau
2018-06-15  4:46     ` John Fastabend
2018-06-14 16:44 ` [bpf PATCH v2 2/6] bpf: sockmap only allow ESTABLISHED sock state John Fastabend
2018-06-15  0:18   ` Martin KaFai Lau
2018-06-18 14:50     ` John Fastabend
2018-06-18 21:17       ` Martin KaFai Lau
2018-06-20 22:15         ` John Fastabend
2018-06-14 16:44 ` [bpf PATCH v2 3/6] bpf: sockhash fix omitted bucket lock in sock_close John Fastabend
2018-06-15  5:41   ` Martin KaFai Lau
2018-06-15 15:23     ` John Fastabend
2018-06-15 15:45       ` Martin KaFai Lau
2018-06-14 16:45 ` [bpf PATCH v2 4/6] bpf: sockmap, tcp_disconnect to listen transition John Fastabend
2018-06-15  6:04   ` Martin KaFai Lau
2018-06-14 16:45 ` [bpf PATCH v2 5/6] bpf: sockhash, add release routine John Fastabend
2018-06-15  6:05   ` Martin KaFai Lau
2018-06-14 16:45 ` [bpf PATCH v2 6/6] bpf: selftest remove attempts to add LISTEN sockets to sockmap John Fastabend
2018-06-15  6:07   ` Martin KaFai Lau

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=20180614164148.24994.65250.stgit@john-Precision-Tower-5810 \
    --to=john.fastabend@gmail.com \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.