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 udpate
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]/
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.
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
This replaces 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'.
Signed-off-by: Arseniy Krasnov <[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
There is no sense to update skbuff state, because it is used only once
after dequeue to copy data and then will be released.
Signed-off-by: Arseniy Krasnov <[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()'.
Signed-off-by: Arseniy Krasnov <[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]>
---
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 Thu, Mar 09, 2023 at 01:10:36PM +0300, Arseniy Krasnov 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 udpate
> 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]/
>
>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.
Yes, but they solve the problem, so we should use the tag (I think at
least in patch 1 and 3).
We usually use the tag when we are fixing a problem introduced by a
previous change. So we need to backport the patch to the stable branches
as well, and we need the tag to figure out which branches have the patch
or not.
Thanks,
Stefano
>
>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
>
On 09.03.2023 19:21, Stefano Garzarella wrote:
> On Thu, Mar 09, 2023 at 01:10:36PM +0300, Arseniy Krasnov 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 udpate
>> 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]/
>>
>> 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.
>
> Yes, but they solve the problem, so we should use the tag (I think at
> least in patch 1 and 3).
>
> We usually use the tag when we are fixing a problem introduced by a
> previous change. So we need to backport the patch to the stable branches
> as well, and we need the tag to figure out which branches have the patch
> or not.
Ahh, sorry. Ok. I see now :)
Thanks, Arseniy
>
> Thanks,
> Stefano
>
>>
>> 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
>>
>
On Thu, Mar 09, 2023 at 01:11:46PM +0300, Arseniy Krasnov wrote:
>This replaces 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'.
I think we should also describe the issues you found that we are fixinig
now, for example the wrong calculation in virtio_transport_dec_rx_pkt().
Something like this:
`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 ...
(continue with what is written in this commit message)
And we should add the Fixes tag:
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>
>Signed-off-by: Arseniy Krasnov <[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);
Good catch! In my proposal I used `bytes` wrongly!
The rest LGTM!
Stefano
>+
>+ 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
>
On Thu, Mar 09, 2023 at 01:12:42PM +0300, Arseniy Krasnov wrote:
I would add:
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.
>
>Signed-off-by: Arseniy Krasnov <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 1 -
> 1 file changed, 1 deletion(-)
The patch LGTM!
Stefano
>
>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
>
On Thu, Mar 09, 2023 at 01:13:51PM +0300, Arseniy Krasnov wrote:
>This returns behaviour of SOCK_STREAM read as before skbuff usage. When
So we need the Fixes tag:
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
The patch LGTM!
Stefano
>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()'.
>
>Signed-off-by: Arseniy Krasnov <[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
>
On Thu, Mar 09, 2023 at 01:14:48PM +0300, Arseniy Krasnov wrote:
>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]>
>---
> tools/testing/vsock/vsock_test.c | 118 +++++++++++++++++++++++++++++++
> 1 file changed, 118 insertions(+)
Reviewed-by: Stefano Garzarella <[email protected]>
>
>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 Thu, Mar 09, 2023 at 07:20:20PM +0300, Arseniy Krasnov wrote:
>
>
>On 09.03.2023 19:21, Stefano Garzarella wrote:
>> On Thu, Mar 09, 2023 at 01:10:36PM +0300, Arseniy Krasnov 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 udpate
>>> ? 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]/
>>>
>>> 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.
>>
>> Yes, but they solve the problem, so we should use the tag (I think at
>> least in patch 1 and 3).
>>
>> We usually use the tag when we are fixing a problem introduced by a
>> previous change. So we need to backport the patch to the stable branches
>> as well, and we need the tag to figure out which branches have the patch
>> or not.
>Ahh, sorry. Ok. I see now :)
No problem at all :-)
I think also patch 2 can have the Fixes tag.
Thanks,
Stefano
>
>Thanks, Arseniy
>>
>> Thanks,
>> Stefano
>>
>>>
>>> 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
>>>
>>
>
On 09.03.2023 19:32, Stefano Garzarella wrote:
> On Thu, Mar 09, 2023 at 07:20:20PM +0300, Arseniy Krasnov wrote:
>>
>>
>> On 09.03.2023 19:21, Stefano Garzarella wrote:
>>> On Thu, Mar 09, 2023 at 01:10:36PM +0300, Arseniy Krasnov 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 udpate
>>>> 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]/
>>>>
>>>> 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.
>>>
>>> Yes, but they solve the problem, so we should use the tag (I think at
>>> least in patch 1 and 3).
>>>
>>> We usually use the tag when we are fixing a problem introduced by a
>>> previous change. So we need to backport the patch to the stable branches
>>> as well, and we need the tag to figure out which branches have the patch
>>> or not.
>> Ahh, sorry. Ok. I see now :)
>
> No problem at all :-)
>
> I think also patch 2 can have the Fixes tag.
>
Done, fixed everything in v4.
Thanks, Arseniy
> Thanks,
> Stefano
>
>>
>> Thanks, Arseniy
>>>
>>> Thanks,
>>> Stefano
>>>
>>>>
>>>> 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
>>>>
>>>
>>
>