netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH RESEND net v4 1/4] virtio/vsock: don't use skbuff state to account credit
  2023-03-14 11:05 ` [PATCH RESEND net v4 1/4] virtio/vsock: don't use skbuff state to account credit Arseniy Krasnov
@ 2023-03-08 15:11   ` Bobby Eshleman
  0 siblings, 0 replies; 9+ messages in thread
From: Bobby Eshleman @ 2023-03-08 15:11 UTC (permalink / raw)
  To: Arseniy Krasnov
  Cc: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman, kvm,
	virtualization, netdev, linux-kernel, kernel, oxffffaa

On Tue, Mar 14, 2023 at 02:05:48PM +0300, Arseniy Krasnov wrote:
> 'skb->len' can vary when we partially read the data, this complicates the
> calculation of credit to be updated in 'virtio_transport_inc_rx_pkt()/
> virtio_transport_dec_rx_pkt()'.
> 
> Also in 'virtio_transport_dec_rx_pkt()' we were miscalculating the
> credit since 'skb->len' was redundant.
> 
> For these reasons, let's replace the use of skbuff state to calculate new
> 'rx_bytes'/'fwd_cnt' values with explicit value as input argument. This
> makes code more simple, because it is not needed to change skbuff state
> before each call to update 'rx_bytes'/'fwd_cnt'.
> 
> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  net/vmw_vsock/virtio_transport_common.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index a1581c77cf84..618680fd9906 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,9 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>  		skb_pull(skb, bytes);
>  
>  		if (skb->len == 0) {
> -			virtio_transport_dec_rx_pkt(vvs, skb);
> +			u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
> +
> +			virtio_transport_dec_rx_pkt(vvs, pkt_len);
>  			consume_skb(skb);
>  		} else {
>  			__skb_queue_head(&vvs->rx_queue, skb);
> @@ -437,17 +436,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 +483,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 +1039,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;
> -- 
> 2.25.1

Acked-by: Bobby Eshleman <bobby.eshleman@bytedance.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net v4 2/4] virtio/vsock: remove redundant 'skb_pull()' call
  2023-03-14 11:06 ` [PATCH RESEND net v4 2/4] virtio/vsock: remove redundant 'skb_pull()' call Arseniy Krasnov
@ 2023-03-08 15:15   ` Bobby Eshleman
  0 siblings, 0 replies; 9+ messages in thread
From: Bobby Eshleman @ 2023-03-08 15:15 UTC (permalink / raw)
  To: Arseniy Krasnov
  Cc: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman, kvm,
	virtualization, netdev, linux-kernel, kernel, oxffffaa

On Tue, Mar 14, 2023 at 02:06:53PM +0300, Arseniy Krasnov wrote:
> Since we now no longer use 'skb->len' to update credit, there is no sense
> to update skbuff state, because it is used only once after dequeue to
> copy data and then will be released.
> 
> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  net/vmw_vsock/virtio_transport_common.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 618680fd9906..9a411475e201 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -465,7 +465,6 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
>  					dequeued_len = err;
>  				} else {
>  					user_buf_len -= bytes_to_copy;
> -					skb_pull(skb, bytes_to_copy);
>  				}
>  
>  				spin_lock_bh(&vvs->rx_lock);
> -- 
> 2.25.1

Acked-by: Bobby Eshleman <bobby.eshleman@bytedance.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net v4 3/4] virtio/vsock: don't drop skbuff on copy failure
  2023-03-14 11:08 ` [PATCH RESEND net v4 3/4] virtio/vsock: don't drop skbuff on copy failure Arseniy Krasnov
@ 2023-03-08 15:16   ` Bobby Eshleman
  0 siblings, 0 replies; 9+ messages in thread
From: Bobby Eshleman @ 2023-03-08 15:16 UTC (permalink / raw)
  To: Arseniy Krasnov
  Cc: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman, kvm,
	virtualization, netdev, linux-kernel, kernel, oxffffaa

On Tue, Mar 14, 2023 at 02:08:20PM +0300, Arseniy Krasnov wrote:
> This returns behaviour of SOCK_STREAM read as before skbuff usage. When
> copying to user fails current skbuff won't be dropped, but returned to
> sockets's queue. Technically instead of 'skb_dequeue()', 'skb_peek()' is
> called and when skbuff becomes empty, it is removed from queue by
> '__skb_unlink()'.
> 
> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  net/vmw_vsock/virtio_transport_common.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 9a411475e201..6564192e7f20 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -364,7 +364,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>  
>  	spin_lock_bh(&vvs->rx_lock);
>  	while (total < len && !skb_queue_empty(&vvs->rx_queue)) {
> -		skb = __skb_dequeue(&vvs->rx_queue);
> +		skb = skb_peek(&vvs->rx_queue);
>  
>  		bytes = len - total;
>  		if (bytes > skb->len)
> @@ -388,9 +388,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>  			u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
>  
>  			virtio_transport_dec_rx_pkt(vvs, pkt_len);
> +			__skb_unlink(skb, &vvs->rx_queue);
>  			consume_skb(skb);
> -		} else {
> -			__skb_queue_head(&vvs->rx_queue, skb);
>  		}
>  	}
>  
> -- 
> 2.25.1

Acked-by: Bobby Eshleman <bobby.eshleman@bytedance.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RESEND net v4 0/4] several updates to virtio/vsock
@ 2023-03-14 11:03 Arseniy Krasnov
  2023-03-14 11:05 ` [PATCH RESEND net v4 1/4] virtio/vsock: don't use skbuff state to account credit Arseniy Krasnov
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Arseniy Krasnov @ 2023-03-14 11:03 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman
  Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa,
	avkrasnov

Hello,

this patchset evolved from previous v2 version (see link below). It does
several updates to virtio/vsock:
1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of
   using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt'
   and 'rx_bytes', integer value is passed as an input argument. This
   makes code more simple, because in this case we don't need to update
   skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In
   more common words - we don't need to change skbuff state to update
   'rx_bytes' and 'fwd_cnt' correctly.
2) For SOCK_STREAM, when copying data to user fails, current skbuff is
   not dropped. Next read attempt will use same skbuff and last offset.
   Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used.
   This behaviour was implemented before skbuff support.
3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for
   this type of socket each skbuff is used only once: after removing it
   from socket's queue, it will be freed anyway.

Test for 2) also added:
Test tries to 'recv()' data to NULL buffer, then does 'recv()' with valid
buffer. For SOCK_STREAM second 'recv()' must return data, because skbuff
must not be dropped, but for SOCK_SEQPACKET skbuff will be dropped by
kernel, and 'recv()' will return EAGAIN.

Link to v1 on lore:
https://lore.kernel.org/netdev/c2d3e204-89d9-88e9-8a15-3fe027e56b4b@sberdevices.ru/

Link to v2 on lore:
https://lore.kernel.org/netdev/a7ab414b-5e41-c7b6-250b-e8401f335859@sberdevices.ru/

Link to v3 on lore:
https://lore.kernel.org/netdev/0abeec42-a11d-3a51-453b-6acf76604f2e@sberdevices.ru/

Change log:

v1 -> v2:
 - For SOCK_SEQPACKET call 'skb_pull()' also in case of copy failure or
   dropping skbuff (when we just waiting message end).
 - Handle copy failure for SOCK_STREAM in the same manner (plus free
   current skbuff).
 - Replace bug repdroducer with new test in vsock_test.c

v2 -> v3:
 - Replace patch which removes 'skb->len' subtraction from function
   'virtio_transport_dec_rx_pkt()' with patch which updates functions
   'virtio_transport_inc/dec_rx_pkt()' by passing integer argument
   instead of skbuff pointer.
 - Replace patch which drops skbuff when copying to user fails with
   patch which changes this behaviour by keeping skbuff in queue until
   it has no data.
 - Add patch for SOCK_SEQPACKET which removes redundant 'skb_pull()'
   call on read.
 - I remove "Fixes" tag from all patches, because all of them now change
   code logic, not only fix something.

v3 -> v4:
 - Update commit messages in all patches except test.
 - Add "Fixes" tag to all patches except test.

Arseniy Krasnov (4):
  virtio/vsock: don't use skbuff state to account credit
  virtio/vsock: remove redundant 'skb_pull()' call
  virtio/vsock: don't drop skbuff on copy failure
  test/vsock: copy to user failure test

 net/vmw_vsock/virtio_transport_common.c |  29 +++---
 tools/testing/vsock/vsock_test.c        | 118 ++++++++++++++++++++++++
 2 files changed, 131 insertions(+), 16 deletions(-)

-- 
2.25.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RESEND net v4 1/4] virtio/vsock: don't use skbuff state to account credit
  2023-03-14 11:03 [PATCH RESEND net v4 0/4] several updates to virtio/vsock Arseniy Krasnov
@ 2023-03-14 11:05 ` Arseniy Krasnov
  2023-03-08 15:11   ` Bobby Eshleman
  2023-03-14 11:06 ` [PATCH RESEND net v4 2/4] virtio/vsock: remove redundant 'skb_pull()' call Arseniy Krasnov
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Arseniy Krasnov @ 2023-03-14 11:05 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman
  Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa,
	avkrasnov

'skb->len' can vary when we partially read the data, this complicates the
calculation of credit to be updated in 'virtio_transport_inc_rx_pkt()/
virtio_transport_dec_rx_pkt()'.

Also in 'virtio_transport_dec_rx_pkt()' we were miscalculating the
credit since 'skb->len' was redundant.

For these reasons, let's replace the use of skbuff state to calculate new
'rx_bytes'/'fwd_cnt' values with explicit value as input argument. This
makes code more simple, because it is not needed to change skbuff state
before each call to update 'rx_bytes'/'fwd_cnt'.

Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/vmw_vsock/virtio_transport_common.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index a1581c77cf84..618680fd9906 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,9 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
 		skb_pull(skb, bytes);
 
 		if (skb->len == 0) {
-			virtio_transport_dec_rx_pkt(vvs, skb);
+			u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
+
+			virtio_transport_dec_rx_pkt(vvs, pkt_len);
 			consume_skb(skb);
 		} else {
 			__skb_queue_head(&vvs->rx_queue, skb);
@@ -437,17 +436,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 +483,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 +1039,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;
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RESEND net v4 2/4] virtio/vsock: remove redundant 'skb_pull()' call
  2023-03-14 11:03 [PATCH RESEND net v4 0/4] several updates to virtio/vsock Arseniy Krasnov
  2023-03-14 11:05 ` [PATCH RESEND net v4 1/4] virtio/vsock: don't use skbuff state to account credit Arseniy Krasnov
@ 2023-03-14 11:06 ` Arseniy Krasnov
  2023-03-08 15:15   ` Bobby Eshleman
  2023-03-14 11:08 ` [PATCH RESEND net v4 3/4] virtio/vsock: don't drop skbuff on copy failure Arseniy Krasnov
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Arseniy Krasnov @ 2023-03-14 11:06 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman
  Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa,
	avkrasnov

Since we now no longer use 'skb->len' to update credit, there is no sense
to update skbuff state, because it is used only once after dequeue to
copy data and then will be released.

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

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 618680fd9906..9a411475e201 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -465,7 +465,6 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
 					dequeued_len = err;
 				} else {
 					user_buf_len -= bytes_to_copy;
-					skb_pull(skb, bytes_to_copy);
 				}
 
 				spin_lock_bh(&vvs->rx_lock);
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RESEND net v4 3/4] virtio/vsock: don't drop skbuff on copy failure
  2023-03-14 11:03 [PATCH RESEND net v4 0/4] several updates to virtio/vsock Arseniy Krasnov
  2023-03-14 11:05 ` [PATCH RESEND net v4 1/4] virtio/vsock: don't use skbuff state to account credit Arseniy Krasnov
  2023-03-14 11:06 ` [PATCH RESEND net v4 2/4] virtio/vsock: remove redundant 'skb_pull()' call Arseniy Krasnov
@ 2023-03-14 11:08 ` Arseniy Krasnov
  2023-03-08 15:16   ` Bobby Eshleman
  2023-03-14 11:09 ` [PATCH RESEND net v4 4/4] test/vsock: copy to user failure test Arseniy Krasnov
  2023-03-16 17:30 ` [PATCH RESEND net v4 0/4] several updates to virtio/vsock patchwork-bot+netdevbpf
  4 siblings, 1 reply; 9+ messages in thread
From: Arseniy Krasnov @ 2023-03-14 11:08 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman
  Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa,
	avkrasnov

This returns behaviour of SOCK_STREAM read as before skbuff usage. When
copying to user fails current skbuff won't be dropped, but returned to
sockets's queue. Technically instead of 'skb_dequeue()', 'skb_peek()' is
called and when skbuff becomes empty, it is removed from queue by
'__skb_unlink()'.

Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/vmw_vsock/virtio_transport_common.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 9a411475e201..6564192e7f20 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -364,7 +364,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
 
 	spin_lock_bh(&vvs->rx_lock);
 	while (total < len && !skb_queue_empty(&vvs->rx_queue)) {
-		skb = __skb_dequeue(&vvs->rx_queue);
+		skb = skb_peek(&vvs->rx_queue);
 
 		bytes = len - total;
 		if (bytes > skb->len)
@@ -388,9 +388,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
 			u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
 
 			virtio_transport_dec_rx_pkt(vvs, pkt_len);
+			__skb_unlink(skb, &vvs->rx_queue);
 			consume_skb(skb);
-		} else {
-			__skb_queue_head(&vvs->rx_queue, skb);
 		}
 	}
 
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RESEND net v4 4/4] test/vsock: copy to user failure test
  2023-03-14 11:03 [PATCH RESEND net v4 0/4] several updates to virtio/vsock Arseniy Krasnov
                   ` (2 preceding siblings ...)
  2023-03-14 11:08 ` [PATCH RESEND net v4 3/4] virtio/vsock: don't drop skbuff on copy failure Arseniy Krasnov
@ 2023-03-14 11:09 ` Arseniy Krasnov
  2023-03-16 17:30 ` [PATCH RESEND net v4 0/4] several updates to virtio/vsock patchwork-bot+netdevbpf
  4 siblings, 0 replies; 9+ messages in thread
From: Arseniy Krasnov @ 2023-03-14 11:09 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Bobby Eshleman
  Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa,
	avkrasnov

This adds SOCK_STREAM and SOCK_SEQPACKET tests for invalid buffer case.
It tries to read data to NULL buffer (data already presents in socket's
queue), then uses valid buffer. For SOCK_STREAM second read must return
data, because skbuff is not dropped, but for SOCK_SEQPACKET skbuff will
be dropped by kernel, and 'recv()' will return EAGAIN.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tools/testing/vsock/vsock_test.c | 118 +++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 67e9f9df3a8c..3de10dbb50f5 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -860,6 +860,114 @@ static void test_stream_poll_rcvlowat_client(const struct test_opts *opts)
 	close(fd);
 }
 
+#define INV_BUF_TEST_DATA_LEN 512
+
+static void test_inv_buf_client(const struct test_opts *opts, bool stream)
+{
+	unsigned char data[INV_BUF_TEST_DATA_LEN] = {0};
+	ssize_t ret;
+	int fd;
+
+	if (stream)
+		fd = vsock_stream_connect(opts->peer_cid, 1234);
+	else
+		fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
+
+	if (fd < 0) {
+		perror("connect");
+		exit(EXIT_FAILURE);
+	}
+
+	control_expectln("SENDDONE");
+
+	/* Use invalid buffer here. */
+	ret = recv(fd, NULL, sizeof(data), 0);
+	if (ret != -1) {
+		fprintf(stderr, "expected recv(2) failure, got %zi\n", ret);
+		exit(EXIT_FAILURE);
+	}
+
+	if (errno != ENOMEM) {
+		fprintf(stderr, "unexpected recv(2) errno %d\n", errno);
+		exit(EXIT_FAILURE);
+	}
+
+	ret = recv(fd, data, sizeof(data), MSG_DONTWAIT);
+
+	if (stream) {
+		/* For SOCK_STREAM we must continue reading. */
+		if (ret != sizeof(data)) {
+			fprintf(stderr, "expected recv(2) success, got %zi\n", ret);
+			exit(EXIT_FAILURE);
+		}
+		/* Don't check errno in case of success. */
+	} else {
+		/* For SOCK_SEQPACKET socket's queue must be empty. */
+		if (ret != -1) {
+			fprintf(stderr, "expected recv(2) failure, got %zi\n", ret);
+			exit(EXIT_FAILURE);
+		}
+
+		if (errno != EAGAIN) {
+			fprintf(stderr, "unexpected recv(2) errno %d\n", errno);
+			exit(EXIT_FAILURE);
+		}
+	}
+
+	control_writeln("DONE");
+
+	close(fd);
+}
+
+static void test_inv_buf_server(const struct test_opts *opts, bool stream)
+{
+	unsigned char data[INV_BUF_TEST_DATA_LEN] = {0};
+	ssize_t res;
+	int fd;
+
+	if (stream)
+		fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL);
+	else
+		fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
+
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	res = send(fd, data, sizeof(data), 0);
+	if (res != sizeof(data)) {
+		fprintf(stderr, "unexpected send(2) result %zi\n", res);
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln("SENDDONE");
+
+	control_expectln("DONE");
+
+	close(fd);
+}
+
+static void test_stream_inv_buf_client(const struct test_opts *opts)
+{
+	test_inv_buf_client(opts, true);
+}
+
+static void test_stream_inv_buf_server(const struct test_opts *opts)
+{
+	test_inv_buf_server(opts, true);
+}
+
+static void test_seqpacket_inv_buf_client(const struct test_opts *opts)
+{
+	test_inv_buf_client(opts, false);
+}
+
+static void test_seqpacket_inv_buf_server(const struct test_opts *opts)
+{
+	test_inv_buf_server(opts, false);
+}
+
 static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM connection reset",
@@ -920,6 +1028,16 @@ static struct test_case test_cases[] = {
 		.run_client = test_seqpacket_bigmsg_client,
 		.run_server = test_seqpacket_bigmsg_server,
 	},
+	{
+		.name = "SOCK_STREAM test invalid buffer",
+		.run_client = test_stream_inv_buf_client,
+		.run_server = test_stream_inv_buf_server,
+	},
+	{
+		.name = "SOCK_SEQPACKET test invalid buffer",
+		.run_client = test_seqpacket_inv_buf_client,
+		.run_server = test_seqpacket_inv_buf_server,
+	},
 	{},
 };
 
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND net v4 0/4] several updates to virtio/vsock
  2023-03-14 11:03 [PATCH RESEND net v4 0/4] several updates to virtio/vsock Arseniy Krasnov
                   ` (3 preceding siblings ...)
  2023-03-14 11:09 ` [PATCH RESEND net v4 4/4] test/vsock: copy to user failure test Arseniy Krasnov
@ 2023-03-16 17:30 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-03-16 17:30 UTC (permalink / raw)
  To: Arseniy Krasnov
  Cc: stefanha, sgarzare, davem, edumazet, kuba, pabeni, bobby.eshleman,
	kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa,
	avkrasnov

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 14 Mar 2023 14:03:23 +0300 you wrote:
> Hello,
> 
> this patchset evolved from previous v2 version (see link below). It does
> several updates to virtio/vsock:
> 1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of
>    using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt'
>    and 'rx_bytes', integer value is passed as an input argument. This
>    makes code more simple, because in this case we don't need to update
>    skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In
>    more common words - we don't need to change skbuff state to update
>    'rx_bytes' and 'fwd_cnt' correctly.
> 2) For SOCK_STREAM, when copying data to user fails, current skbuff is
>    not dropped. Next read attempt will use same skbuff and last offset.
>    Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used.
>    This behaviour was implemented before skbuff support.
> 3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for
>    this type of socket each skbuff is used only once: after removing it
>    from socket's queue, it will be freed anyway.
> 
> [...]

Here is the summary with links:
  - [RESEND,net,v4,1/4] virtio/vsock: don't use skbuff state to account credit
    https://git.kernel.org/netdev/net/c/077706165717
  - [RESEND,net,v4,2/4] virtio/vsock: remove redundant 'skb_pull()' call
    https://git.kernel.org/netdev/net/c/6825e6b4f8e5
  - [RESEND,net,v4,3/4] virtio/vsock: don't drop skbuff on copy failure
    https://git.kernel.org/netdev/net/c/8daaf39f7f6e
  - [RESEND,net,v4,4/4] test/vsock: copy to user failure test
    https://git.kernel.org/netdev/net/c/7e699d2a4e81

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-03-16 17:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-14 11:03 [PATCH RESEND net v4 0/4] several updates to virtio/vsock Arseniy Krasnov
2023-03-14 11:05 ` [PATCH RESEND net v4 1/4] virtio/vsock: don't use skbuff state to account credit Arseniy Krasnov
2023-03-08 15:11   ` Bobby Eshleman
2023-03-14 11:06 ` [PATCH RESEND net v4 2/4] virtio/vsock: remove redundant 'skb_pull()' call Arseniy Krasnov
2023-03-08 15:15   ` Bobby Eshleman
2023-03-14 11:08 ` [PATCH RESEND net v4 3/4] virtio/vsock: don't drop skbuff on copy failure Arseniy Krasnov
2023-03-08 15:16   ` Bobby Eshleman
2023-03-14 11:09 ` [PATCH RESEND net v4 4/4] test/vsock: copy to user failure test Arseniy Krasnov
2023-03-16 17:30 ` [PATCH RESEND net v4 0/4] several updates to virtio/vsock patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).