[PATCH bpf 0/4] bpf, vsock: Fixes related to sockmap/sockhash redirection

[PATCH bpf 0/4] bpf, vsock: Fixes related to sockmap/sockhash redirection

[Michal Luczaj]

Series consists of few fixes for issues uncovered while working on a BPF
sockmap/sockhash redirection selftest.

The last patch is more of a RFC clean up attempt. Patch claims that there's
no functional change, but effectively it removes (never touched?) reference
to sock_map_unhash().

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Michal Luczaj (4):
      bpf, sockmap: SK_DROP on attempted redirects of unsupported af_vsock
      vsock: Update rx_bytes on read_skb()
      vsock: Update msg_count on read_skb()
      bpf, vsock: Drop static vsock_bpf_prot initialization

 include/net/sock.h                      |  5 +++++
 net/core/sock_map.c                     |  8 ++++++++
 net/vmw_vsock/virtio_transport_common.c | 14 +++++++++++---
 net/vmw_vsock/vsock_bpf.c               |  8 --------
 4 files changed, 24 insertions(+), 11 deletions(-)
---
base-commit: 94e354adf6c210ce79827f5affb0cf69f380d181
change-id: 20241009-vsock-fixes-for-redir-86707e1e8c04

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

[PATCH bpf 1/4] bpf, sockmap: SK_DROP on attempted redirects of unsupported af_vsock

[Michal Luczaj]

Don't mislead the callers of bpf_{sk,msg}_redirect_{map,hash}(): make sure
to immediately and visibly fail the forwarding of unsupported af_vsock
packets.

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 include/net/sock.h  | 5 +++++
 net/core/sock_map.c | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/net/sock.h b/include/net/sock.h
index c58ca8dd561b7312ffc0836585c04d9fe917a124..c87295f3476db23934d4fcbeabc7851c61ad2bc4 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 242c91a6e3d3870ec6da6fa095d180a933d1d3d4..07d6aa4e39ef606aab33bd0d95711ecf156596b9 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;

-- 
2.46.2

[PATCH bpf 2/4] vsock: Update rx_bytes on read_skb()

[Michal Luczaj]

Make sure virtio_transport_inc_rx_pkt() and virtio_transport_dec_rx_pkt()
calls are balanced (i.e. virtio_vsock_sock::rx_bytes doesn't lie) after
vsock_transport::read_skb().

Failing to update rx_bytes after packet is dequeued leads to a warning on
SOCK_STREAM recv():

[  233.396654] rx_queue is empty, but rx_bytes is non-zero
[  233.396702] WARNING: CPU: 11 PID: 40601 at net/vmw_vsock/virtio_transport_common.c:589

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 884ee128851e5ce8b01c78fcb95a408986f62936..ed1c1bed5700e5988a233cea146cf9fac95426e0 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1707,6 +1707,7 @@ int virtio_transport_read_skb(struct vsock_sock *vsk, skb_read_actor_t recv_acto
 {
 	struct virtio_vsock_sock *vvs = vsk->trans;
 	struct sock *sk = sk_vsock(vsk);
+	struct virtio_vsock_hdr *hdr;
 	struct sk_buff *skb;
 	int off = 0;
 	int err;
@@ -1716,10 +1717,14 @@ int virtio_transport_read_skb(struct vsock_sock *vsk, skb_read_actor_t recv_acto
 	 * works for types other than dgrams.
 	 */
 	skb = __skb_recv_datagram(sk, &vvs->rx_queue, MSG_DONTWAIT, &off, &err);
-	spin_unlock_bh(&vvs->rx_lock);
-
-	if (!skb)
+	if (!skb) {
+		spin_unlock_bh(&vvs->rx_lock);
 		return err;
+	}
+
+	hdr = virtio_vsock_hdr(skb);
+	virtio_transport_dec_rx_pkt(vvs, le32_to_cpu(hdr->len));
+	spin_unlock_bh(&vvs->rx_lock);
 
 	return recv_actor(sk, skb);
 }

-- 
2.46.2

[PATCH bpf 3/4] vsock: Update msg_count on read_skb()

[Michal Luczaj]

Dequeuing via vsock_transport::read_skb() left msg_count outdated, which
then confused SOCK_SEQPACKET recv(). Decrease the counter.

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index ed1c1bed5700e5988a233cea146cf9fac95426e0..1d591b69ede3244a4f49aa44dc1f939d827dafc0 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1723,6 +1723,9 @@ int virtio_transport_read_skb(struct vsock_sock *vsk, skb_read_actor_t recv_acto
 	}
 
 	hdr = virtio_vsock_hdr(skb);
+	if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SEQ_EOM)
+		vvs->msg_count--;
+
 	virtio_transport_dec_rx_pkt(vvs, le32_to_cpu(hdr->len));
 	spin_unlock_bh(&vvs->rx_lock);
 

-- 
2.46.2

[PATCH bpf 4/4] bpf, vsock: Drop static vsock_bpf_prot initialization

[Michal Luczaj]

vsock_bpf_prot is set up at runtime. Remove the superfluous init.

No functional change intended.

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/vsock_bpf.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
index c42c5cc18f324108e044772e957c8d42c92ead8c..4aa6e74ec2957b28b9e9d8ce0b5f4d5c289a9276 100644
--- a/net/vmw_vsock/vsock_bpf.c
+++ b/net/vmw_vsock/vsock_bpf.c
@@ -114,14 +114,6 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
 	return copied;
 }
 
-/* Copy of original proto with updated sock_map methods */
-static struct proto vsock_bpf_prot = {
-	.close = sock_map_close,
-	.recvmsg = vsock_bpf_recvmsg,
-	.sock_is_readable = sk_msg_is_readable,
-	.unhash = sock_map_unhash,
-};
-
 static void vsock_bpf_rebuild_protos(struct proto *prot, const struct proto *base)
 {
 	*prot        = *base;

-- 
2.46.2

Re: [PATCH bpf 2/4] vsock: Update rx_bytes on read_skb()

[Stefano Garzarella]

On Wed, Oct 09, 2024 at 11:20:51PM GMT, Michal Luczaj wrote:
>Make sure virtio_transport_inc_rx_pkt() and virtio_transport_dec_rx_pkt()
>calls are balanced (i.e. virtio_vsock_sock::rx_bytes doesn't lie) after
>vsock_transport::read_skb().
>
>Failing to update rx_bytes after packet is dequeued leads to a warning on
>SOCK_STREAM recv():
>
>[  233.396654] rx_queue is empty, but rx_bytes is non-zero
>[  233.396702] WARNING: CPU: 11 PID: 40601 at net/vmw_vsock/virtio_transport_common.c:589
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)


The modification looks good to me, but now that I'm looking at it 
better, I don't understand why we don't also call 
virtio_transport_send_credit_update().

This is to inform the peer that we've freed up space and it has more 
credit.

@Bobby do you remember?

I think we should try to unify the receiving path used through BPF or 
not (not for this series of course).

Thanks,
Stefano

>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 884ee128851e5ce8b01c78fcb95a408986f62936..ed1c1bed5700e5988a233cea146cf9fac95426e0 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1707,6 +1707,7 @@ int virtio_transport_read_skb(struct vsock_sock *vsk, skb_read_actor_t recv_acto
> {
> 	struct virtio_vsock_sock *vvs = vsk->trans;
> 	struct sock *sk = sk_vsock(vsk);
>+	struct virtio_vsock_hdr *hdr;
> 	struct sk_buff *skb;
> 	int off = 0;
> 	int err;
>@@ -1716,10 +1717,14 @@ int virtio_transport_read_skb(struct vsock_sock *vsk, skb_read_actor_t recv_acto
> 	 * works for types other than dgrams.
> 	 */
> 	skb = __skb_recv_datagram(sk, &vvs->rx_queue, MSG_DONTWAIT, &off, &err);
>-	spin_unlock_bh(&vvs->rx_lock);
>-
>-	if (!skb)
>+	if (!skb) {
>+		spin_unlock_bh(&vvs->rx_lock);
> 		return err;
>+	}
>+
>+	hdr = virtio_vsock_hdr(skb);
>+	virtio_transport_dec_rx_pkt(vvs, le32_to_cpu(hdr->len));
>+	spin_unlock_bh(&vvs->rx_lock);
>
> 	return recv_actor(sk, skb);
> }
>
>-- 
>2.46.2
>

Re: [PATCH bpf 3/4] vsock: Update msg_count on read_skb()

[Stefano Garzarella]

On Wed, Oct 09, 2024 at 11:20:52PM GMT, Michal Luczaj wrote:
>Dequeuing via vsock_transport::read_skb() left msg_count outdated, which
>then confused SOCK_SEQPACKET recv(). Decrease the counter.
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 3 +++
> 1 file changed, 3 insertions(+)

Thanks for fixing this!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index ed1c1bed5700e5988a233cea146cf9fac95426e0..1d591b69ede3244a4f49aa44dc1f939d827dafc0 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1723,6 +1723,9 @@ int virtio_transport_read_skb(struct vsock_sock *vsk, skb_read_actor_t recv_acto
> 	}
>
> 	hdr = virtio_vsock_hdr(skb);
>+	if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SEQ_EOM)
>+		vvs->msg_count--;
>+
> 	virtio_transport_dec_rx_pkt(vvs, le32_to_cpu(hdr->len));
> 	spin_unlock_bh(&vvs->rx_lock);
>
>
>-- 
>2.46.2
>

Re: [PATCH bpf 0/4] bpf, vsock: Fixes related to sockmap/sockhash redirection

[Stefano Garzarella]

On Wed, Oct 09, 2024 at 11:20:49PM GMT, Michal Luczaj wrote:
>Series consists of few fixes for issues uncovered while working on a BPF
>sockmap/sockhash redirection selftest.
>
>The last patch is more of a RFC clean up attempt. Patch claims that there's
>no functional change, but effectively it removes (never touched?) reference
>to sock_map_unhash().

I'm not an expert on BPF, so I reviewed only those that touch 
virtio-vsock.

I only have a doubt about another call to add, otherwise LGTM.

I think we should handle the two receive paths better, so in the future 
we should refactor the code a bit to avoid this difference. I'll try to 
take a look at it in the coming weeks, but for now let's get on with 
this series.

Thanks,
Stefano

>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
>Michal Luczaj (4):
>      bpf, sockmap: SK_DROP on attempted redirects of unsupported af_vsock
>      vsock: Update rx_bytes on read_skb()
>      vsock: Update msg_count on read_skb()
>      bpf, vsock: Drop static vsock_bpf_prot initialization
>
> include/net/sock.h                      |  5 +++++
> net/core/sock_map.c                     |  8 ++++++++
> net/vmw_vsock/virtio_transport_common.c | 14 +++++++++++---
> net/vmw_vsock/vsock_bpf.c               |  8 --------
> 4 files changed, 24 insertions(+), 11 deletions(-)
>---
>base-commit: 94e354adf6c210ce79827f5affb0cf69f380d181
>change-id: 20241009-vsock-fixes-for-redir-86707e1e8c04
>
>Best regards,
>-- 
>Michal Luczaj <mhal@rbox.co>
>

Re: [PATCH bpf 1/4] bpf, sockmap: SK_DROP on attempted redirects of unsupported af_vsock

[Martin KaFai Lau]

On 10/9/24 2:20 PM, Michal Luczaj wrote:
> Don't mislead the callers of bpf_{sk,msg}_redirect_{map,hash}(): make sure
> to immediately and visibly fail the forwarding of unsupported af_vsock
> packets.
> 
> Fixes: 634f1a7110b4 ("vsock: support sockmap")
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>   include/net/sock.h  | 5 +++++
>   net/core/sock_map.c | 8 ++++++++
>   2 files changed, 13 insertions(+)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index c58ca8dd561b7312ffc0836585c04d9fe917a124..c87295f3476db23934d4fcbeabc7851c61ad2bc4 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 242c91a6e3d3870ec6da6fa095d180a933d1d3d4..07d6aa4e39ef606aab33bd0d95711ecf156596b9 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;

Jakub Sitnicki, I think you have been on another thread about this change. 
Please help to take a look and ack if it looks good. Thanks.

>   
>   	msg->flags = flags;
>   	msg->sk_redir = sk;
> 

Re: [External] Re: [PATCH bpf 2/4] vsock: Update rx_bytes on read_skb()

[Stefano Garzarella]

On Thu, Oct 10, 2024 at 05:09:17PM GMT, Robert Eshleman . wrote:
>On Thu, Oct 10, 2024 at 1:49 AM Stefano Garzarella <sgarzare@redhat.com>
>wrote:
>
>>
>> The modification looks good to me, but now that I'm looking at it
>> better, I don't understand why we don't also call
>> virtio_transport_send_credit_update().
>>
>> This is to inform the peer that we've freed up space and it has more
>> credit.
>>
>> @Bobby do you remember?
>>
>>
>I do not remember, but I do think it seems wrong not to.

Yeah, @Michal can you also add that call?

For now just call it, without the optimization we did for stream 
packets, in the future I'll try to unify the paths.

Thanks,
Stefano

>
>
>> I think we should try to unify the receiving path used through BPF or
>> not (not for this series of course).
>>
>> Thanks,
>> Stefano
>>

Re: [External] Re: [PATCH bpf 2/4] vsock: Update rx_bytes on read_skb()

[Michal Luczaj]

On 10/11/24 10:40, Stefano Garzarella wrote:
> On Thu, Oct 10, 2024 at 05:09:17PM GMT, Robert Eshleman . wrote:
>> On Thu, Oct 10, 2024 at 1:49 AM Stefano Garzarella <sgarzare@redhat.com>
>> wrote:
>>>
>>> The modification looks good to me, but now that I'm looking at it
>>> better, I don't understand why we don't also call
>>> virtio_transport_send_credit_update().
>>>
>>> This is to inform the peer that we've freed up space and it has more
>>> credit.
>>>
>>> @Bobby do you remember?
>>>
>>>
>> I do not remember, but I do think it seems wrong not to.
> 
> Yeah, @Michal can you also add that call?
> 
> For now just call it, without the optimization we did for stream 
> packets, in the future I'll try to unify the paths.

Sure, here is v2:
https://lore.kernel.org/bpf/20241013-vsock-fixes-for-redir-v2-0-d6577bbfe742@rbox.co

Thanks,
Michal