* [RFC PATCH v1 0/3] virtio/vsock: fix credit update logic
@ 2023-03-03 21:57 Arseniy Krasnov
2023-03-03 22:00 ` [RFC PATCH v1 1/3] test/vsock: SOCK_SEQPACKET 'rx_bytes'/'fwd_cnt' bug reproducer Arseniy Krasnov
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Arseniy Krasnov @ 2023-03-03 21:57 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 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC PATCH v1 1/3] test/vsock: SOCK_SEQPACKET 'rx_bytes'/'fwd_cnt' bug reproducer
2023-03-03 21:57 [RFC PATCH v1 0/3] virtio/vsock: fix credit update logic Arseniy Krasnov
@ 2023-03-03 22:00 ` Arseniy Krasnov
2023-03-03 22:01 ` [RFC PATCH v1 2/3] virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation Arseniy Krasnov
2023-03-03 22:02 ` [RFC PATCH v1 3/3] virtio/vsock: remove all data from sk_buff Arseniy Krasnov
2 siblings, 0 replies; 5+ messages in thread
From: Arseniy Krasnov @ 2023-03-03 22:00 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
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v1 2/3] virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation
2023-03-03 21:57 [RFC PATCH v1 0/3] virtio/vsock: fix credit update logic Arseniy Krasnov
2023-03-03 22:00 ` [RFC PATCH v1 1/3] test/vsock: SOCK_SEQPACKET 'rx_bytes'/'fwd_cnt' bug reproducer Arseniy Krasnov
@ 2023-03-03 22:01 ` Arseniy Krasnov
2023-03-03 22:02 ` [RFC PATCH v1 3/3] virtio/vsock: remove all data from sk_buff Arseniy Krasnov
2 siblings, 0 replies; 5+ messages in thread
From: Arseniy Krasnov @ 2023-03-03 22:01 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
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v1 3/3] virtio/vsock: remove all data from sk_buff
2023-03-03 21:57 [RFC PATCH v1 0/3] virtio/vsock: fix credit update logic Arseniy Krasnov
2023-03-03 22:00 ` [RFC PATCH v1 1/3] test/vsock: SOCK_SEQPACKET 'rx_bytes'/'fwd_cnt' bug reproducer Arseniy Krasnov
2023-03-03 22:01 ` [RFC PATCH v1 2/3] virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation Arseniy Krasnov
@ 2023-03-03 22:02 ` Arseniy Krasnov
[not found] ` <CALa-AnCu8g+jt1m_rY0QJFcRUhtWJ64Txro69j9KsnK7hyuBMg@mail.gmail.com>
2 siblings, 1 reply; 5+ messages in thread
From: Arseniy Krasnov @ 2023-03-03 22:02 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
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [External] [RFC PATCH v1 3/3] virtio/vsock: remove all data from sk_buff
[not found] ` <CALa-AnCu8g+jt1m_rY0QJFcRUhtWJ64Txro69j9KsnK7hyuBMg@mail.gmail.com>
@ 2023-03-04 11:53 ` Arseniy Krasnov
0 siblings, 0 replies; 5+ messages in thread
From: Arseniy Krasnov @ 2023-03-04 11:53 UTC (permalink / raw)
To: Robert Eshleman .
Cc: Stefan Hajnoczi, Stefano Garzarella, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, kvm, virtualization,
netdev, linux-kernel, kernel, oxffffaa
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-04 11:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-03 21:57 [RFC PATCH v1 0/3] virtio/vsock: fix credit update logic Arseniy Krasnov
2023-03-03 22:00 ` [RFC PATCH v1 1/3] test/vsock: SOCK_SEQPACKET 'rx_bytes'/'fwd_cnt' bug reproducer Arseniy Krasnov
2023-03-03 22:01 ` [RFC PATCH v1 2/3] virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation Arseniy Krasnov
2023-03-03 22:02 ` [RFC PATCH v1 3/3] virtio/vsock: remove all data from sk_buff Arseniy Krasnov
[not found] ` <CALa-AnCu8g+jt1m_rY0QJFcRUhtWJ64Txro69j9KsnK7hyuBMg@mail.gmail.com>
2023-03-04 11:53 ` [External] " Arseniy Krasnov
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).