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;
