[RFC,v2,1/4] virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation

Message ID 4a3f3978-1093-4c0a-663f-28d77eeb0806@sberdevices.ru
State New
Headers
Series virtio/vsock: fix credit update logic |

Commit Message

Arseniy Krasnov March 5, 2023, 8:06 p.m. UTC
  Substraction of 'skb->len' is redundant here: 'skb_headroom()' is delta
between 'data' and 'head' pointers, e.g. it is number of bytes returned
to user (of course accounting size of header). 'skb->len' is number of
bytes rest in buffer.

Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 net/vmw_vsock/virtio_transport_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Stefano Garzarella March 6, 2023, 11:57 a.m. UTC | #1
On Sun, Mar 05, 2023 at 11:06:26PM +0300, Arseniy Krasnov wrote:
>Substraction of 'skb->len' is redundant here: 'skb_headroom()' is delta
>between 'data' and 'head' pointers, e.g. it is number of bytes returned
>to user (of course accounting size of header). 'skb->len' is number of
>bytes rest in buffer.
>
>Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index a1581c77cf84..2e2a773df5c1 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -255,7 +255,7 @@ static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs,
> {
> 	int len;
>
>-	len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;
>+	len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr);

IIUC virtio_transport_dec_rx_pkt() is always called after skb_pull(),
so skb_headroom() is returning the amount of space we removed.

Looking at the other patches in this series, I think maybe we should
change virtio_transport_dec_rx_pkt() and virtio_transport_inc_rx_pkt()
by passing the value to subtract or add directly.
Since some times we don't remove the whole payload, so it would be
better to call it with the value in hdr->len.

I mean something like this (untested):

index a1581c77cf84..9e69ae7a9a96 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -241,21 +241,18 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
  }

  static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
-                                       struct sk_buff *skb)
+                                       u32 len)
  {
-       if (vvs->rx_bytes + skb->len > vvs->buf_alloc)
+       if (vvs->rx_bytes + len > vvs->buf_alloc)
                 return false;

-       vvs->rx_bytes += skb->len;
+       vvs->rx_bytes += len;
         return true;
  }

  static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs,
-                                       struct sk_buff *skb)
+                                       u32 len)
  {
-       int len;
-
-       len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;
         vvs->rx_bytes -= len;
         vvs->fwd_cnt += len;
  }
@@ -388,7 +385,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
                 skb_pull(skb, bytes);

                 if (skb->len == 0) {
-                       virtio_transport_dec_rx_pkt(vvs, skb);
+                       virtio_transport_dec_rx_pkt(vvs, bytes);
                         consume_skb(skb);
                 } else {
                         __skb_queue_head(&vvs->rx_queue, skb);
@@ -437,17 +434,17 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,

         while (!msg_ready) {
                 struct virtio_vsock_hdr *hdr;
+               size_t pkt_len;

                 skb = __skb_dequeue(&vvs->rx_queue);
                 if (!skb)
                         break;
                 hdr = virtio_vsock_hdr(skb);
+               pkt_len = (size_t)le32_to_cpu(hdr->len);

                 if (dequeued_len >= 0) {
-                       size_t pkt_len;
                         size_t bytes_to_copy;

-                       pkt_len = (size_t)le32_to_cpu(hdr->len);
                         bytes_to_copy = min(user_buf_len, pkt_len);

                         if (bytes_to_copy) {
@@ -484,7 +481,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
                                 msg->msg_flags |= MSG_EOR;
                 }

-               virtio_transport_dec_rx_pkt(vvs, skb);
+               virtio_transport_dec_rx_pkt(vvs, pkt_len);
                 kfree_skb(skb);
         }

@@ -1040,7 +1037,7 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,

         spin_lock_bh(&vvs->rx_lock);

-       can_enqueue = virtio_transport_inc_rx_pkt(vvs, skb);
+       can_enqueue = virtio_transport_inc_rx_pkt(vvs, len);
         if (!can_enqueue) {
                 free_pkt = true;
                 goto out;

When we used vsock_pkt, we were passing the structure because the `len`
field was immutable (and copied from the header), whereas with skb it
can change and so we introduced these errors.

What do you think?

Thanks,
Stefano
  
Arseniy Krasnov March 6, 2023, 3:27 p.m. UTC | #2
On 06.03.2023 14:57, Stefano Garzarella wrote:
> On Sun, Mar 05, 2023 at 11:06:26PM +0300, Arseniy Krasnov wrote:
>> Substraction of 'skb->len' is redundant here: 'skb_headroom()' is delta
>> between 'data' and 'head' pointers, e.g. it is number of bytes returned
>> to user (of course accounting size of header). 'skb->len' is number of
>> bytes rest in buffer.
>>
>> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index a1581c77cf84..2e2a773df5c1 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -255,7 +255,7 @@ static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs,
>> {
>>     int len;
>>
>> -    len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;
>> +    len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr);
> 
> IIUC virtio_transport_dec_rx_pkt() is always called after skb_pull(),
> so skb_headroom() is returning the amount of space we removed.
> 
> Looking at the other patches in this series, I think maybe we should
> change virtio_transport_dec_rx_pkt() and virtio_transport_inc_rx_pkt()
> by passing the value to subtract or add directly.
> Since some times we don't remove the whole payload, so it would be
> better to call it with the value in hdr->len.
> 
> I mean something like this (untested):
> 
> index a1581c77cf84..9e69ae7a9a96 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -241,21 +241,18 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>  }
> 
>  static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
> -                                       struct sk_buff *skb)
> +                                       u32 len)
>  {
> -       if (vvs->rx_bytes + skb->len > vvs->buf_alloc)
> +       if (vvs->rx_bytes + len > vvs->buf_alloc)
>                 return false;
> 
> -       vvs->rx_bytes += skb->len;
> +       vvs->rx_bytes += len;
>         return true;
>  }
> 
>  static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs,
> -                                       struct sk_buff *skb)
> +                                       u32 len)
>  {
> -       int len;
> -
> -       len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;
>         vvs->rx_bytes -= len;
>         vvs->fwd_cnt += len;
>  }
> @@ -388,7 +385,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>                 skb_pull(skb, bytes);
> 
>                 if (skb->len == 0) {
> -                       virtio_transport_dec_rx_pkt(vvs, skb);
> +                       virtio_transport_dec_rx_pkt(vvs, bytes);
>                         consume_skb(skb);
>                 } else {
>                         __skb_queue_head(&vvs->rx_queue, skb);
> @@ -437,17 +434,17 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
> 
>         while (!msg_ready) {
>                 struct virtio_vsock_hdr *hdr;
> +               size_t pkt_len;
> 
>                 skb = __skb_dequeue(&vvs->rx_queue);
>                 if (!skb)
>                         break;
>                 hdr = virtio_vsock_hdr(skb);
> +               pkt_len = (size_t)le32_to_cpu(hdr->len);
> 
>                 if (dequeued_len >= 0) {
> -                       size_t pkt_len;
>                         size_t bytes_to_copy;
> 
> -                       pkt_len = (size_t)le32_to_cpu(hdr->len);
>                         bytes_to_copy = min(user_buf_len, pkt_len);
> 
>                         if (bytes_to_copy) {
> @@ -484,7 +481,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
>                                 msg->msg_flags |= MSG_EOR;
>                 }
> 
> -               virtio_transport_dec_rx_pkt(vvs, skb);
> +               virtio_transport_dec_rx_pkt(vvs, pkt_len);
>                 kfree_skb(skb);
>         }
> 
> @@ -1040,7 +1037,7 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
> 
>         spin_lock_bh(&vvs->rx_lock);
> 
> -       can_enqueue = virtio_transport_inc_rx_pkt(vvs, skb);
> +       can_enqueue = virtio_transport_inc_rx_pkt(vvs, len);
>         if (!can_enqueue) {
>                 free_pkt = true;
>                 goto out;
> 
> When we used vsock_pkt, we were passing the structure because the `len`
> field was immutable (and copied from the header), whereas with skb it
> can change and so we introduced these errors.
> 
> What do you think?
Yes, i think passing explicit integer value to 'virtio_transport_inc/dec_rx_pkt'
is more clear solution. I had this variant, but thought that current will be
smaller. Current version with skb argument forces to call 'skb_pull()' before
each 'virtio_transport_dec_rx_pkt()' as 'rx_bytes'/'fwd_cnt' new value relies
on skb parameters - otherwise 'rx_bytes' become invalid. 'skb_pull()' will be
used only to update 'skb->len' which shows rest of the data. I'll do it in v3.

Thanks, Arseniy
> 
> Thanks,
> Stefano
>
  

Patch

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index a1581c77cf84..2e2a773df5c1 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -255,7 +255,7 @@  static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs,
 {
 	int len;
 
-	len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;
+	len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr);
 	vvs->rx_bytes -= len;
 	vvs->fwd_cnt += len;
 }