Re: [PATCH bpf-next v2 0/6] selftests/bpf: Various sockmap-related fixes

[Michal Luczaj]

On 9/24/24 12:25, Michal Luczaj wrote:
> On 8/19/24 22:05, Jakub Sitnicki wrote:
>> On Wed, Aug 14, 2024 at 06:14 PM +02, Michal Luczaj wrote:
>>> On 8/6/24 19:45, Jakub Sitnicki wrote:
>>>> On Tue, Aug 06, 2024 at 07:18 PM +02, Michal Luczaj wrote:
>>>>> Great, thanks for the review. With this completed, I guess we can unwind
>>>>> the (mail) stack to [1]. Is that ingress-to-local et al. something you
>>>>> wanted to take care of yourself or can I give it a try?
>>>>> [1] https://lore.kernel.org/netdev/87msmqn9ws.fsf@xxxxxxxxxxxxxx/
>>>>
>>>> I haven't stated any work on. You're welcome to tackle that.
>>>>
>>>> All I have is a toy test that I've used to generate the redirect matrix.
>>>> Perhaps it can serve as inspiration:
>>>>
>>>> https://github.com/jsitnicki/sockmap-redir-matrix
>>>
>>> All right, please let me know if this is more or less what you meant and
>>> I'll post the whole series for a review (+patch to purge sockmap_listen of
>>> redir tests, fix misnomers). [...]
>>
>> Gave it a look as promised. It makes sense to me as well to put these
>> tests in a new module. There will be some overlap with sockmap_listen,
>> which has diverged from its inital scope, but we can dedup that later.
>>
>> One thought that I had is that it could make sense to test the not
>> supported redirect combos (and expect an error). Sometimes folks make
>> changes and enable some parts of the API by accient.
> 
> All right, so I did what sockmap_listen does: check
> test_sockmap_listen.c:verdict_map[SK_PASS] to see if the redirect took
> place for a given combo. And that works well... except for skb/msg to
> ingress af_vsock. Even though this is unsupported and no redirect
> actually happens, verdict appears to be SK_PASS. Is this correct?

Here's a follow up: my guess is that some checks are missing. I'm not sure
if it's the best approach, but this fixes things for me:

diff --git a/include/net/sock.h b/include/net/sock.h
index c58ca8dd561b..c87295f3476d 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2715,6 +2715,11 @@ static inline bool sk_is_stream_unix(const struct sock *sk)
 	return sk->sk_family == AF_UNIX && sk->sk_type == SOCK_STREAM;
 }
 
+static inline bool sk_is_vsock(const struct sock *sk)
+{
+	return sk->sk_family == AF_VSOCK;
+}
+
 /**
  * sk_eat_skb - Release a skb if it is no longer needed
  * @sk: socket to eat this skb from
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 242c91a6e3d3..07d6aa4e39ef 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -647,6 +647,8 @@ BPF_CALL_4(bpf_sk_redirect_map, struct sk_buff *, skb,
 	sk = __sock_map_lookup_elem(map, key);
 	if (unlikely(!sk || !sock_map_redirect_allowed(sk)))
 		return SK_DROP;
+	if ((flags & BPF_F_INGRESS) && sk_is_vsock(sk))
+		return SK_DROP;
 
 	skb_bpf_set_redir(skb, sk, flags & BPF_F_INGRESS);
 	return SK_PASS;
@@ -675,6 +677,8 @@ BPF_CALL_4(bpf_msg_redirect_map, struct sk_msg *, msg,
 		return SK_DROP;
 	if (!(flags & BPF_F_INGRESS) && !sk_is_tcp(sk))
 		return SK_DROP;
+	if (sk_is_vsock(sk))
+		return SK_DROP;
 
 	msg->flags = flags;
 	msg->sk_redir = sk;
@@ -1249,6 +1253,8 @@ BPF_CALL_4(bpf_sk_redirect_hash, struct sk_buff *, skb,
 	sk = __sock_hash_lookup_elem(map, key);
 	if (unlikely(!sk || !sock_map_redirect_allowed(sk)))
 		return SK_DROP;
+	if ((flags & BPF_F_INGRESS) && sk_is_vsock(sk))
+		return SK_DROP;
 
 	skb_bpf_set_redir(skb, sk, flags & BPF_F_INGRESS);
 	return SK_PASS;
@@ -1277,6 +1283,8 @@ BPF_CALL_4(bpf_msg_redirect_hash, struct sk_msg *, msg,
 		return SK_DROP;
 	if (!(flags & BPF_F_INGRESS) && !sk_is_tcp(sk))
 		return SK_DROP;
+	if (sk_is_vsock(sk))
+		return SK_DROP;
 
 	msg->flags = flags;
 	msg->sk_redir = sk;