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

[Jakub Sitnicki <jakub@cloudflare.com>]

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.fsf@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;