[RFC PATCH v1 0/3] virtio/vsock: fix credit update logic

[RFC PATCH v1 0/3] virtio/vsock: fix credit update logic

[Arseniy Krasnov]

Hello,

this patchset fixes two things in credit account logic:
1) Current implementation of 'virtio_transport_dec_rx_pkt()':

   value to update 'rx_bytes' and 'fwd_cnt' is calculated as:

   skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;

   i'm a little bit confused about subtracting 'skb->len'. It is clear,
   that difference between first two components is number of bytes copied
   to user. 'skb_headroom()' is delta between 'data' and 'head'. 'data'
   is incremented on each copy data to user from skb by call 'skb_pull()'
   (at the same moment, 'skb->len' is decremented to the same amount of
   bytes). 'head' points to the header of the packet. But what is purpose
   of 'skb->len' here? For SOCK_STREAM is has no effect because this
   logic is called only when 'skb->len' == 0, but for SOCK_SEQPACKET and
   other future calls i think it is buggy.

2) For SOCK_SEQPACKET all sk_buffs are handled only once - after dequeue
   each sk_buff is removed, so user will never read rest of the data.
   Thus we need to update credit parameters of the socket ('rx_bytes' and
   'fwd_cnt') like whole sk_buff is read - so call 'skb_pull()' for the
   whole buffer.

Reproducer is included. To trigger problem run vsock_test without two
patches with fix - You will see 'Negative len:'. Patches with fixes
depends on reproducer due to 'pr_emerg()', but i can resend them, seems
not a big deal.


Arseniy Krasnov (3):
  test/vsock: SOCK_SEQPACKET 'rx_bytes'/'fwd_cnt' bug reproducer
  virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation
  virtio/vsock: remove all data from sk_buff

 net/vmw_vsock/virtio_transport_common.c |  8 +++--
 tools/testing/vsock/vsock_test.c        | 44 +++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

-- 
2.25.1

[RFC PATCH v1 1/3] test/vsock: SOCK_SEQPACKET 'rx_bytes'/'fwd_cnt' bug reproducer

[Arseniy Krasnov]

virtio/vsock: replace virtio_vsock_pkt with sk_buff

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 net/vmw_vsock/virtio_transport_common.c |  4 +++
 tools/testing/vsock/vsock_test.c        | 44 +++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index a1581c77cf84..77bb1cad8471 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -256,6 +256,10 @@ 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;
+
+	if (len < 0)
+		pr_emerg("Negative len %i\n", len);
+
 	vvs->rx_bytes -= len;
 	vvs->fwd_cnt += len;
 }
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 67e9f9df3a8c..2651de2aedc9 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -860,7 +860,51 @@ static void test_stream_poll_rcvlowat_client(const struct test_opts *opts)
 	close(fd);
 }
 
+static void test_seqpacket_rxbytes_client(const struct test_opts *opts)
+{
+	unsigned char data[256];
+	int fd;
+
+	fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
+	if (fd < 0) {
+		perror("connect");
+		exit(EXIT_FAILURE);
+	}
+
+	send(fd, data, sizeof(data), 0);
+
+	control_writeln("CLISENT");
+
+	close(fd);
+
+	exit(0);
+}
+
+static void test_seqpacket_rxbytes_server(const struct test_opts *opts)
+{
+	unsigned char data[8];
+	int fd;
+
+	fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	control_expectln("CLISENT");
+	read(fd, data, sizeof(data));
+
+	close(fd);
+
+	exit(0);
+}
+
 static struct test_case test_cases[] = {
+	{
+		.name = "SOCK_SEQPACKET negative 'rx_bytes'",
+		.run_client = test_seqpacket_rxbytes_client,
+		.run_server = test_seqpacket_rxbytes_server,
+	},
 	{
 		.name = "SOCK_STREAM connection reset",
 		.run_client = test_stream_connection_reset,
-- 
2.25.1

[RFC PATCH v1 2/3] virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation

[Arseniy Krasnov]

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 77bb1cad8471..d80075e1db42 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);
 
 	if (len < 0)
 		pr_emerg("Negative len %i\n", len);
-- 
2.25.1

[RFC PATCH v1 3/3] virtio/vsock: remove all data from sk_buff

[Arseniy Krasnov]

In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
data from it, it will be removed, so user will never read rest of the
data. Thus we need to update credit parameters of the socket like whole
sk_buff is read - so call 'skb_pull()' for the whole 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 d80075e1db42..bbcf331b6ad6 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -470,7 +470,7 @@ 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);
+					skb_pull(skb, skb->len);
 				}
 
 				spin_lock_bh(&vvs->rx_lock);
-- 
2.25.1

Re: [External] [RFC PATCH v1 3/3] virtio/vsock: remove all data from sk_buff

[Arseniy Krasnov]

On 04.03.2023 02:00, Robert Eshleman . wrote:
> On Fri, Mar 3, 2023 at 2:05 PM Arseniy Krasnov <avkrasnov@sberdevices.ru>
> wrote:
> 
>> In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
>> data from it, it will be removed, so user will never read rest of the
>> data. Thus we need to update credit parameters of the socket like whole
>> sk_buff is read - so call 'skb_pull()' for the whole 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 d80075e1db42..bbcf331b6ad6 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -470,7 +470,7 @@ 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);
>> +                                       skb_pull(skb, skb->len);
>>                                 }
>>
>>
> I believe this may also need to be done when memcpy_to_msg() returns an
> error.
Hello! Thanks for quick reply. Yes, moreover  in case of SEQPACKET 'skb_pull()' must be called
every time when skbuff was removed from queue - it doesn't matter did we copy data from, get
error on memcpy_to_msg(), or just drop it - otherwise we get leak of 'rx_bytes'.

Also in case of STREAM, skb_pull() must be called for the rest of data in skbuff in case of error,
because again - 'rx_bytes' will leak.

I think, i'll prepare fixes and tests for this case in the next week

Thanks, Arseniy
> 
> Best,
> Bobby
>