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/[email protected]/
Link to v2 on lore:
https://lore.kernel.org/netdev/[email protected]/
Link to v3 on lore:
https://lore.kernel.org/netdev/[email protected]/
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
'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 <[email protected]>
Reviewed-by: Stefano Garzarella <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Stefano Garzarella <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Stefano Garzarella <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Stefano Garzarella <[email protected]>
---
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
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 <[email protected]>
> Reviewed-by: Stefano Garzarella <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> Reviewed-by: Stefano Garzarella <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> Reviewed-by: Stefano Garzarella <[email protected]>
> ---
> 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 <[email protected]>
Hello:
This series was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
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