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

[PATCH bpf v2 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>
---
Changes in v2:
- Patch 2/4: Send a credit update [Stefano]
- Collect Reviewed-by
- Link to v1: https://lore.kernel.org/r/20241009-vsock-fixes-for-redir-v1-0-e455416f6d78@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, 25 insertions(+), 10 deletions(-)
---
base-commit: afeb2b51a761c9c52be5639eb40460462083f222
change-id: 20241009-vsock-fixes-for-redir-86707e1e8c04

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

[PATCH bpf v2 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 v2 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().

While here, also inform the peer that we've freed up space and it has more
credit.

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")
Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/virtio_transport_common.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 884ee128851e5ce8b01c78fcb95a408986f62936..2e5ad96825cc0988c9e1b3f8a8bfcff2ef00a2b2 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,16 @@ 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);
+	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);
 
-	if (!skb)
-		return err;
+	virtio_transport_send_credit_update(vsk);
 
 	return recv_actor(sk, skb);
 }

-- 
2.46.2

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

[Stefano Garzarella]

On Sun, Oct 13, 2024 at 06:26:40PM +0200, 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().
>
>While here, also inform the peer that we've freed up space and it has more
>credit.
>
>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")
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/virtio_transport_common.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)

Thanks for fixing this!

LGTM:
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 884ee128851e5ce8b01c78fcb95a408986f62936..2e5ad96825cc0988c9e1b3f8a8bfcff2ef00a2b2 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,16 @@ 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);
>+	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);
>
>-	if (!skb)
>-		return err;
>+	virtio_transport_send_credit_update(vsk);
>
> 	return recv_actor(sk, skb);
> }
>
>-- 
>2.46.2
>

[PATCH bpf v2 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")
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
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 2e5ad96825cc0988c9e1b3f8a8bfcff2ef00a2b2..ccbd2bc0d2109aea4f19e79a0438f85893e1d89c 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 v2 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 v2 0/4] bpf, vsock: Fixes related to sockmap/sockhash redirection

[Stefano Garzarella]

On Sun, Oct 13, 2024 at 06:26:38PM +0200, 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().
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
>Changes in v2:
>- Patch 2/4: Send a credit update [Stefano]
>- Collect Reviewed-by
>- Link to v1: https://lore.kernel.org/r/20241009-vsock-fixes-for-redir-v1-0-e455416f6d78@rbox.co

For the virtio-vsock point of view, the series LGTM and I reviewed patch 
2 and 3. I don't know BPF enough for the rest but I can't see anything 
wrong.

Thanks,
Stefano

>
>---
>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, 25 insertions(+), 10 deletions(-)
>---
>base-commit: afeb2b51a761c9c52be5639eb40460462083f222
>change-id: 20241009-vsock-fixes-for-redir-86707e1e8c04
>
>Best regards,
>-- 
>Michal Luczaj <mhal@rbox.co>
>

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

[John Fastabend]

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().
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

For the series LGTM, ack.

Acked-by: John Fastabend <john.fastabend@gmail.com>

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Sun, 13 Oct 2024 18:26:38 +0200 you 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().
> 
> [...]

Here is the summary with links:
  - [bpf,v2,1/4] bpf, sockmap: SK_DROP on attempted redirects of unsupported af_vsock
    https://git.kernel.org/bpf/bpf/c/9c5bd93edf7b
  - [bpf,v2,2/4] vsock: Update rx_bytes on read_skb()
    https://git.kernel.org/bpf/bpf/c/3543152f2d33
  - [bpf,v2,3/4] vsock: Update msg_count on read_skb()
    https://git.kernel.org/bpf/bpf/c/6dafde852df8
  - [bpf,v2,4/4] bpf, vsock: Drop static vsock_bpf_prot initialization
    https://git.kernel.org/bpf/bpf/c/19039f279797

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html