diff mbox series

[bpf,1/2] bpf, sockmap: Attach map progs to psock early for feature probes

Message ID 20211119181418.353932-2-john.fastabend@gmail.com (mailing list archive)
State Accepted
Commit 38207a5e81230d6ffbdd51e5fa5681be5116dcae
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, 26 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 TCP socket is added to a sock map we look at the programs attached
to the map to determine what proto op hooks need to be changed. Before
the patch in the 'fixes' tag there were only two categories -- the empty
set of programs or a TX policy. In any case the base set handled the
receive case.

After the fix we have an optimized program for receive that closes a
small, but possible, race on receive. This program is loaded only
when the map the psock is being added to includes a RX policy.
Otherwise, the race is not possible so we don't need to handle the
race condition.

In order for the call to sk_psock_init() to correctly evaluate the
above conditions all progs need to be set in the psock before the
call. However, in the current code this is not the case. We end
up evaluating the requirements on the old prog state. If your psock
is attached to multiple maps -- for example a tx map and rx map --
then the second update would pull in the correct maps. But, the
other pattern with a single rx enabled map the correct receive
hooks are not used. The result is the race fixed by the patch in
the fixes tag below may still be seen in this case.

To fix we simply set all psock->progs before doing the call into
sock_map_init((). With this the init() call gets the full list
of programs and chooses the correct proto ops on the first
iteration instead of requiring the second update to pull them
in. This fixes the race case when only a single map is used.

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/sock_map.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
diff mbox series

Patch

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index f39ef79ced67..9b528c644fb7 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -282,6 +282,12 @@  static int sock_map_link(struct bpf_map *map, struct sock *sk)
 
 	if (msg_parser)
 		psock_set_prog(&psock->progs.msg_parser, msg_parser);
+	if (stream_parser)
+		psock_set_prog(&psock->progs.stream_parser, stream_parser);
+	if (stream_verdict)
+		psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
+	if (skb_verdict)
+		psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
 
 	ret = sock_map_init_proto(sk, psock);
 	if (ret < 0)
@@ -292,14 +298,10 @@  static int sock_map_link(struct bpf_map *map, struct sock *sk)
 		ret = sk_psock_init_strp(sk, psock);
 		if (ret)
 			goto out_unlock_drop;
-		psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
-		psock_set_prog(&psock->progs.stream_parser, stream_parser);
 		sk_psock_start_strp(sk, psock);
 	} else if (!stream_parser && stream_verdict && !psock->saved_data_ready) {
-		psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
 		sk_psock_start_verdict(sk,psock);
 	} else if (!stream_verdict && skb_verdict && !psock->saved_data_ready) {
-		psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
 		sk_psock_start_verdict(sk, psock);
 	}
 	write_unlock_bh(&sk->sk_callback_lock);