netdev - Re: [PATCH bpf v2 2/4] vsock: Update rx_bytes on read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <beao3to2xe4h3ahidckfcf5upndl7vdeeb4dehqy2mpt42fa34@6d5n44gn3l35>
Date: Tue, 15 Oct 2024 10:29:45 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, John Fastabend <john.fastabend@...il.com>, 
	Jakub Sitnicki <jakub@...udflare.com>, "Michael S. Tsirkin" <mst@...hat.com>, 
	Bobby Eshleman <bobby.eshleman@...edance.com>, Stefan Hajnoczi <stefanha@...hat.com>, netdev@...r.kernel.org, 
	bpf@...r.kernel.org
Subject: Re: [PATCH bpf v2 2/4] vsock: Update rx_bytes on read_skb()

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@...hat.com>
>Signed-off-by: Michal Luczaj <mhal@...x.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@...hat.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
>