diff mbox series

[bpf,2/2] bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap

Message ID 20211119181418.353932-3-john.fastabend@gmail.com (mailing list archive)
State Accepted
Commit c0d95d3380ee099d735e08618c0d599e72f6c8b0
Delegated to: BPF
Headers show
Series sockmap fix for test_map failure | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers fail 1 blamed authors not CCed: jakub@cloudflare.com; 9 maintainers not CCed: kafai@fb.com songliubraving@fb.com kuba@kernel.org yhs@fb.com lmb@cloudflare.com davem@davemloft.net ast@kernel.org jakub@cloudflare.com kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 22 this patch: 22
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 6 this patch: 6
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf fail VM_Test
bpf/vmtest-bpf-PR fail PR summary

Commit Message

John Fastabend Nov. 19, 2021, 6:14 p.m. UTC
When a sock is added to a sock map we evaluate what proto op hooks need
to be used. However, when the program is removed from the sock map
we have not been evaluating if that changes the required program layout.

Before the patch listed in the 'fixes' tag this was not causing failures
because the base program set handles all cases. Specifically, the case
with a stream parser and the case with out a stream parser are both
handled. With the fix below we identified a race when running with a
proto op that attempts to read skbs off both the stream parser and the
skb->receive_queue. Namely, that a race existed where when the stream
parser is empty checking the skb->reqceive_queue from recvmsg at the
precies moment when the parser is paused and the receive_queue is not
empty could result in skipping the stream parser. This may break a
RX policy depending on the parser to run.

The fix tag then loads a specific proto ops that resolved this race.
But, we missed removing that proto ops recv hook when the sock is
removed from the sockmap. The result is the stream parser is stopped
so no more skbs will be aggregated there, but the hook and BPF program
continues to be attached on the psock. User space will then get an
EBUSY when trying to read the socket because the recvmsg() handler
is now waiting on a stopped stream parser.

To fix we rerun the proto ops init() function which will look at the
new set of progs attached to the psock and rest the proto ops hook
to the correct handlers. And in the above case where we remove the
sock from the sock map the RX prog will no longer be listed so the
proto ops is removed.

Fixes: c5d2177a72a16 ("bpf, sockmap: Fix race in ingress receive verdict with redirect to self")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/core/skmsg.c    | 5 +++++
 net/core/sock_map.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)
diff mbox series

Patch

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 1ae52ac943f6..8eb671c827f9 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1124,6 +1124,8 @@  void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
 
 void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
 {
+	psock_set_prog(&psock->progs.stream_parser, NULL);
+
 	if (!psock->saved_data_ready)
 		return;
 
@@ -1212,6 +1214,9 @@  void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
 
 void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
 {
+	psock_set_prog(&psock->progs.stream_verdict, NULL);
+	psock_set_prog(&psock->progs.skb_verdict, NULL);
+
 	if (!psock->saved_data_ready)
 		return;
 
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 9b528c644fb7..4ca4b11f4e5f 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -167,8 +167,11 @@  static void sock_map_del_link(struct sock *sk,
 		write_lock_bh(&sk->sk_callback_lock);
 		if (strp_stop)
 			sk_psock_stop_strp(sk, psock);
-		else
+		if (verdict_stop)
 			sk_psock_stop_verdict(sk, psock);
+
+		if (psock->psock_update_sk_prot)
+			psock->psock_update_sk_prot(sk, psock, false);
 		write_unlock_bh(&sk->sk_callback_lock);
 	}
 }