On Fri, Sep 27, 2024 at 12:54 AM +02, Michal Luczaj wrote: > 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....@cloudflare.com/ >>>>> >>>>> 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:
So you have already found a bug with a negative test. Nice. Your patch makes sense to me. FYI, I've started a GH repo for anciallary materials for sockmap. Code samples, pointers to resources, a backlog of stuff to do (?). Inspired by the xdp-project repo: https://github.com/xdp-project/xdp-project We can move it to a dedicated project namespace, right now it's at: https://github.com/jsitnicki/sockmap-project/ Feel free to add stuff there. > 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;
