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

Commit Message

Michal Luczaj Oct. 9, 2024, 9:20 p.m. UTC

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 <[e-mail]>
---
 net/vmw_vsock/virtio_transport_common.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Stefano Garzarella Oct. 10, 2024, 8:49 a.m. UTC | #1

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 <[e-mail]>
>---
> 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
>

Stefano Garzarella Oct. 11, 2024, 8:40 a.m. UTC | #2

On Thu, Oct 10, 2024 at 05:09:17PM GMT, Robert Eshleman . wrote:
>On Thu, Oct 10, 2024 at 1:49 AM Stefano Garzarella <[e-mail]>
>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
>>

Michal Luczaj Oct. 13, 2024, 4:28 p.m. UTC | #3

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 <[e-mail]>
>> 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/[e-mail]

Thanks,
Michal

Patch

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);
 }