[PATCH bpf 0/2] sockmap fix for test_map failure

[PATCH bpf 0/2] sockmap fix for test_map failure

[John Fastabend]

CI test_map runs started failing because of a regression in the
sockmap tests. The case, caught by test_maps is that progs attached
to sockets are not detatched currently when sockets are removed
from a map. We resolve this in two patches. The first patch
fixes a subtle issue found from code review and the second
patch addresses the reported CI issue. This was recently introduced
by a race fix, see patches for details.

Sorry for the hassle here, seems we missed ./test_maps run before
pushing the offending patch or maybe we just got lucky on the
run we did locally. Either way should be resolved now.

Thanks,
John

John Fastabend (2):
  bpf, sockmap: Attach map progs to psock early for feature probes
  bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap

 net/core/skmsg.c    |  5 +++++
 net/core/sock_map.c | 15 ++++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

-- 
2.33.0

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

[John Fastabend]

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 --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);
-- 
2.33.0

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

[John Fastabend]

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 --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);
 	}
 }
-- 
2.33.0

Re: [PATCH bpf 0/2] sockmap fix for test_map failure

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri, 19 Nov 2021 10:14:16 -0800 you wrote:
> CI test_map runs started failing because of a regression in the
> sockmap tests. The case, caught by test_maps is that progs attached
> to sockets are not detatched currently when sockets are removed
> from a map. We resolve this in two patches. The first patch
> fixes a subtle issue found from code review and the second
> patch addresses the reported CI issue. This was recently introduced
> by a race fix, see patches for details.
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf, sockmap: Attach map progs to psock early for feature probes
    https://git.kernel.org/bpf/bpf/c/38207a5e8123
  - [bpf,2/2] bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap
    https://git.kernel.org/bpf/bpf/c/c0d95d3380ee

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html