Hello,
this patchset fixes three things in skbuff handling:
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.
3) For SOCK_STREAM when 'memcpy_to_msg()' fails it fixes 'rx_bytes'
update (like in 2)) and frees current skbuff.
Test is also added to vsock_test. It does two attempts to read data from
socket - first attempt to invalid buffer (kernel must drop skb). Second
attempt is performed with valid buffer and MSG_DONTWAIT flag. If socket's
queue will be empty (skbuff was dropped due to 'memcpy_to_msg()' fail
and 'rx_bytes' which controls data waiting set to 0), such call will
return immediately with EAGAIN.
Link to v1 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
Arseniy Krasnov (4):
virtio/vsock: fix 'rx_bytes'/'fwd_cnt' calculation
virtio/vsock: remove all data from sk_buff
virtio/vsock: free skb on data copy failure
test/vsock: invalid buffer tests
net/vmw_vsock/virtio_transport_common.c | 10 ++-
tools/testing/vsock/vsock_test.c | 106 ++++++++++++++++++++++++
2 files changed, 113 insertions(+), 3 deletions(-)
--
2.25.1
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 <[email protected]>
---
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 a1581c77cf84..2e2a773df5c1 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);
vvs->rx_bytes -= len;
vvs->fwd_cnt += len;
}
--
2.25.1
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 <[email protected]>
---
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 2e2a773df5c1..30b0539990ba 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -466,7 +466,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);
@@ -484,6 +483,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
msg->msg_flags |= MSG_EOR;
}
+ skb_pull(skb, skb->len);
virtio_transport_dec_rx_pkt(vvs, skb);
kfree_skb(skb);
}
--
2.25.1
This fixes two things in case when 'memcpy_to_msg()' fails:
1) Update credit parameters of the socket, like this skbuff was
copied to user successfully. This is needed because when skbuff was
received it's length was used to update 'rx_bytes', thus when we drop
skbuff here, we must account rest of it's data in 'rx_bytes'.
2) Free skbuff which was removed from socket's queue.
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 | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 30b0539990ba..ffb1af4f2b52 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -379,8 +379,12 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
spin_unlock_bh(&vvs->rx_lock);
err = memcpy_to_msg(msg, skb->data, bytes);
- if (err)
+ if (err) {
+ skb_pull(skb, skb->len);
+ virtio_transport_dec_rx_pkt(vvs, skb);
+ consume_skb(skb);
goto out;
+ }
spin_lock_bh(&vvs->rx_lock);
--
2.25.1
This adds STREAM and SEQPACKET tests for invalid buffer handling. It
tries to read data to NULL buffer (data already presents in socket's
queue), on fail kernel will free skbuff with data and next read with
MSG_DONTWAIT flag must return EAGAIN which means that skbuff was
dropped and socket's queue is empty.
Signed-off-by: Arseniy Krasnov <[email protected]>
---
tools/testing/vsock/vsock_test.c | 106 +++++++++++++++++++++++++++++++
1 file changed, 106 insertions(+)
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 67e9f9df3a8c..a17794a2d66c 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -860,6 +860,102 @@ static void test_stream_poll_rcvlowat_client(const struct test_opts *opts)
close(fd);
}
+static void test_inv_buf_client(const struct test_opts *opts, bool stream)
+{
+ unsigned char data[512] = {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);
+ }
+
+ /* Now use valid buffer, but socket's queue must be empty. */
+ ret = recv(fd, data, sizeof(data), MSG_DONTWAIT);
+ 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[512] = {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 +1016,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 Sun, Mar 05, 2023 at 11:06:26PM +0300, Arseniy Krasnov wrote:
>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 <[email protected]>
>---
> 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 a1581c77cf84..2e2a773df5c1 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);
IIUC virtio_transport_dec_rx_pkt() is always called after skb_pull(),
so skb_headroom() is returning the amount of space we removed.
Looking at the other patches in this series, I think maybe we should
change virtio_transport_dec_rx_pkt() and virtio_transport_inc_rx_pkt()
by passing the value to subtract or add directly.
Since some times we don't remove the whole payload, so it would be
better to call it with the value in hdr->len.
I mean something like this (untested):
index a1581c77cf84..9e69ae7a9a96 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,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
skb_pull(skb, bytes);
if (skb->len == 0) {
- virtio_transport_dec_rx_pkt(vvs, skb);
+ virtio_transport_dec_rx_pkt(vvs, bytes);
consume_skb(skb);
} else {
__skb_queue_head(&vvs->rx_queue, skb);
@@ -437,17 +434,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 +481,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 +1037,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;
When we used vsock_pkt, we were passing the structure because the `len`
field was immutable (and copied from the header), whereas with skb it
can change and so we introduced these errors.
What do you think?
Thanks,
Stefano
On Sun, Mar 05, 2023 at 11:08:38PM +0300, Arseniy Krasnov wrote:
>This fixes two things in case when 'memcpy_to_msg()' fails:
>1) Update credit parameters of the socket, like this skbuff was
> copied to user successfully. This is needed because when skbuff was
> received it's length was used to update 'rx_bytes', thus when we drop
> skbuff here, we must account rest of it's data in 'rx_bytes'.
>2) Free skbuff which was removed from socket's queue.
>
>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 | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 30b0539990ba..ffb1af4f2b52 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -379,8 +379,12 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
> spin_unlock_bh(&vvs->rx_lock);
>
> err = memcpy_to_msg(msg, skb->data, bytes);
>- if (err)
>+ if (err) {
>+ skb_pull(skb, skb->len);
>+ virtio_transport_dec_rx_pkt(vvs, skb);
>+ consume_skb(skb);
I'm not sure it's the right thing to do, if we fail to copy the content
into the user's buffer, I think we should queue it again.
In fact, before commit 71dc9ec9ac7d ("virtio/vsock: replace
virtio_vsock_pkt with sk_buff"), we used to remove the packet from the
rx_queue, only if memcpy_to_msg() was successful.
Maybe it is better to do as we did before and use skb_peek() at the
beginning of the loop and __skb_unlink() when skb->len == 0.
Thanks,
Stefano
> goto out;
>+ }
>
> spin_lock_bh(&vvs->rx_lock);
>
>--
>2.25.1
>
On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov 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 <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Maybe we could avoid this patch if we directly use pkt_len as I
suggested in the previous patch.
Thanks,
Stefano
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 2e2a773df5c1..30b0539990ba 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -466,7 +466,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);
>@@ -484,6 +483,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
> msg->msg_flags |= MSG_EOR;
> }
>
>+ skb_pull(skb, skb->len);
> virtio_transport_dec_rx_pkt(vvs, skb);
> kfree_skb(skb);
> }
>--
>2.25.1
>
On 06.03.2023 14:57, Stefano Garzarella wrote:
> On Sun, Mar 05, 2023 at 11:06:26PM +0300, Arseniy Krasnov wrote:
>> 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 <[email protected]>
>> ---
>> 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 a1581c77cf84..2e2a773df5c1 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);
>
> IIUC virtio_transport_dec_rx_pkt() is always called after skb_pull(),
> so skb_headroom() is returning the amount of space we removed.
>
> Looking at the other patches in this series, I think maybe we should
> change virtio_transport_dec_rx_pkt() and virtio_transport_inc_rx_pkt()
> by passing the value to subtract or add directly.
> Since some times we don't remove the whole payload, so it would be
> better to call it with the value in hdr->len.
>
> I mean something like this (untested):
>
> index a1581c77cf84..9e69ae7a9a96 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,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
> skb_pull(skb, bytes);
>
> if (skb->len == 0) {
> - virtio_transport_dec_rx_pkt(vvs, skb);
> + virtio_transport_dec_rx_pkt(vvs, bytes);
> consume_skb(skb);
> } else {
> __skb_queue_head(&vvs->rx_queue, skb);
> @@ -437,17 +434,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 +481,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 +1037,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;
>
> When we used vsock_pkt, we were passing the structure because the `len`
> field was immutable (and copied from the header), whereas with skb it
> can change and so we introduced these errors.
>
> What do you think?
Yes, i think passing explicit integer value to 'virtio_transport_inc/dec_rx_pkt'
is more clear solution. I had this variant, but thought that current will be
smaller. Current version with skb argument forces to call 'skb_pull()' before
each 'virtio_transport_dec_rx_pkt()' as 'rx_bytes'/'fwd_cnt' new value relies
on skb parameters - otherwise 'rx_bytes' become invalid. 'skb_pull()' will be
used only to update 'skb->len' which shows rest of the data. I'll do it in v3.
Thanks, Arseniy
>
> Thanks,
> Stefano
>
On 06.03.2023 15:07, Stefano Garzarella wrote:
> On Sun, Mar 05, 2023 at 11:08:38PM +0300, Arseniy Krasnov wrote:
>> This fixes two things in case when 'memcpy_to_msg()' fails:
>> 1) Update credit parameters of the socket, like this skbuff was
>> copied to user successfully. This is needed because when skbuff was
>> received it's length was used to update 'rx_bytes', thus when we drop
>> skbuff here, we must account rest of it's data in 'rx_bytes'.
>> 2) Free skbuff which was removed from socket's queue.
>>
>> 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 | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 30b0539990ba..ffb1af4f2b52 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -379,8 +379,12 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>> spin_unlock_bh(&vvs->rx_lock);
>>
>> err = memcpy_to_msg(msg, skb->data, bytes);
>> - if (err)
>> + if (err) {
>> + skb_pull(skb, skb->len);
>> + virtio_transport_dec_rx_pkt(vvs, skb);
>> + consume_skb(skb);
>
> I'm not sure it's the right thing to do, if we fail to copy the content
> into the user's buffer, I think we should queue it again.
>
> In fact, before commit 71dc9ec9ac7d ("virtio/vsock: replace
> virtio_vsock_pkt with sk_buff"), we used to remove the packet from the
> rx_queue, only if memcpy_to_msg() was successful.
>
> Maybe it is better to do as we did before and use skb_peek() at the
> beginning of the loop and __skb_unlink() when skb->len == 0.
Yes, i see. I'll also add test to cover this case.
>
> Thanks,
> Stefano
>
>> goto out;
>> + }
>>
>> spin_lock_bh(&vvs->rx_lock);
>>
>> --
>> 2.25.1
>>
>
On 06.03.2023 15:08, Stefano Garzarella wrote:
> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov 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 <[email protected]>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Maybe we could avoid this patch if we directly use pkt_len as I
> suggested in the previous patch.
Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
will use integer argument? Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
is never returned to queue to read it again, so i think may be there is no sense for
extra call 'skb_pull'?
Thanks, Arseniy
>
> Thanks,
> Stefano
>
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 2e2a773df5c1..30b0539990ba 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -466,7 +466,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);
>> @@ -484,6 +483,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
>> msg->msg_flags |= MSG_EOR;
>> }
>>
>> + skb_pull(skb, skb->len);
>> virtio_transport_dec_rx_pkt(vvs, skb);
>> kfree_skb(skb);
>> }
>> --
>> 2.25.1
>>
>
On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
>
>
>On 06.03.2023 15:08, Stefano Garzarella wrote:
>> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov 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 <[email protected]>
>>> ---
>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Maybe we could avoid this patch if we directly use pkt_len as I
>> suggested in the previous patch.
>Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
>will use integer argument?
Yep, exactly!
>Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
It depends on how we call virtio_transport_inc_rx_pkt(). If we use
hdr->len there I would use the same to avoid confusion. Plus that's the
value the other peer sent us, so definitely the right value to increase
fwd_cnt with. But if skb->len always reflects it, then that's fine.
>is never returned to queue to read it again, so i think may be there is no sense for
>extra call 'skb_pull'?
Right!
Thanks,
Stefano
On 06.03.2023 18:51, Stefano Garzarella wrote:
> On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
>>
>>
>> On 06.03.2023 15:08, Stefano Garzarella wrote:
>>> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov 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 <[email protected]>
>>>> ---
>>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Maybe we could avoid this patch if we directly use pkt_len as I
>>> suggested in the previous patch.
>> Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
>> will use integer argument?
>
> Yep, exactly!
>
>> Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
>
> It depends on how we call virtio_transport_inc_rx_pkt(). If we use
> hdr->len there I would use the same to avoid confusion. Plus that's the
> value the other peer sent us, so definitely the right value to increase
> fwd_cnt with. But if skb->len always reflects it, then that's fine.
i've checked 'virtio_transport_rx_work()', it calls 'virtio_vsock_skb_rx_put()' which
sets 'skb->len'. Value is used from header, so seems 'skb->len' == 'hdr->len' in this
case.
>
>> is never returned to queue to read it again, so i think may be there is no sense for
>> extra call 'skb_pull'?
>
> Right!
>
> Thanks,
> Stefano
>
On Mon, Mar 06, 2023 at 07:00:10PM +0300, Arseniy Krasnov wrote:
>
>
>On 06.03.2023 18:51, Stefano Garzarella wrote:
>> On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
>>>
>>>
>>> On 06.03.2023 15:08, Stefano Garzarella wrote:
>>>> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov 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 <[email protected]>
>>>>> ---
>>>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> Maybe we could avoid this patch if we directly use pkt_len as I
>>>> suggested in the previous patch.
>>> Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
>>> will use integer argument?
>>
>> Yep, exactly!
>>
>>> Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
>>
>> It depends on how we call virtio_transport_inc_rx_pkt(). If we use
>> hdr->len there I would use the same to avoid confusion. Plus that's the
>> value the other peer sent us, so definitely the right value to increase
>> fwd_cnt with. But if skb->len always reflects it, then that's fine.
>i've checked 'virtio_transport_rx_work()', it calls 'virtio_vsock_skb_rx_put()' which
>sets 'skb->len'. Value is used from header, so seems 'skb->len' == 'hdr->len' in this
>case.
Thank you for checking it.
However, I still think it is better to use `hdr->len` (we have to assign
it to `pkt_len` anyway, as in the proposal I sent for patch 1),
otherwise we have to go every time to check if skb_* functions touch
skb->len.
E.g. skb_pull() decrease skb->len, so I'm not sure we can call
virtio_transport_dec_rx_pkt(skb->len) if we don't remove `skb_pull(skb,
bytes_to_copy);` inside the loop.
Thanks,
Stefano
On Mon, Mar 06, 2023 at 05:18:52PM +0100, Stefano Garzarella wrote:
> On Mon, Mar 06, 2023 at 07:00:10PM +0300, Arseniy Krasnov wrote:
> >
> >
> > On 06.03.2023 18:51, Stefano Garzarella wrote:
> > > On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
> > > >
> > > >
> > > > On 06.03.2023 15:08, Stefano Garzarella wrote:
> > > > > On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov 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 <[email protected]>
> > > > > > ---
> > > > > > net/vmw_vsock/virtio_transport_common.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > Maybe we could avoid this patch if we directly use pkt_len as I
> > > > > suggested in the previous patch.
> > > > Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
> > > > will use integer argument?
> > >
> > > Yep, exactly!
> > >
> > > > Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
> > >
> > > It depends on how we call virtio_transport_inc_rx_pkt(). If we use
> > > hdr->len there I would use the same to avoid confusion. Plus that's the
> > > value the other peer sent us, so definitely the right value to increase
> > > fwd_cnt with. But if skb->len always reflects it, then that's fine.
> > i've checked 'virtio_transport_rx_work()', it calls 'virtio_vsock_skb_rx_put()' which
> > sets 'skb->len'. Value is used from header, so seems 'skb->len' == 'hdr->len' in this
> > case.
>
> Thank you for checking it.
>
> However, I still think it is better to use `hdr->len` (we have to assign it
> to `pkt_len` anyway, as in the proposal I sent for patch 1), otherwise we
> have to go every time to check if skb_* functions touch skb->len.
>
> E.g. skb_pull() decrease skb->len, so I'm not sure we can call
> virtio_transport_dec_rx_pkt(skb->len) if we don't remove `skb_pull(skb,
> bytes_to_copy);` inside the loop.
>
I think it does make reasoning about the bytes accounting easier if it
is based off of the non-mutating hdr->len.
Especially if vsock does ever support tunneling (e.g., through
virtio-net) or some future feature that makes the skb->len more dynamic.
Best,
Bobby