Hello,
This patchset does several things around MSG_PEEK flag support. In
general words it reworks MSG_PEEK test and adds support for this flag
in SOCK_SEQPACKET logic. Here is per-patch description:
1) This is cosmetic change for SOCK_STREAM implementation of MSG_PEEK:
1) I think there is no need of "safe" mode walk here as there is no
"unlink" of skbs inside loop (it is MSG_PEEK mode - we don't change
queue).
2) Nested while loop is removed: in case of MSG_PEEK we just walk
over skbs and copy data from each one. I guess this nested loop
even didn't behave as loop - it always executed just for single
iteration.
2) This adds MSG_PEEK support for SOCK_SEQPACKET. It could be implemented
be reworking MSG_PEEK callback for SOCK_STREAM to support SOCK_SEQPACKET
also, but I think it will be more simple and clear from potential
bugs to implemented it as separate function thus not mixing logics
for both types of socket. So I've added it as dedicated function.
3) This is reworked MSG_PEEK test for SOCK_STREAM. Previous version just
sent single byte, then tried to read it with MSG_PEEK flag, then read
it in normal way. New version is more complex: now sender uses buffer
instead of single byte and this buffer is initialized with random
values. Receiver tests several things:
1) Read empty socket with MSG_PEEK flag.
2) Read part of buffer with MSG_PEEK flag.
3) Read whole buffer with MSG_PEEK flag, then checks that it is same
as buffer from 2) (limited by size of buffer from 2) of course).
4) Read whole buffer without any flags, then checks that is is same
as buffer from 3).
4) This is MSG_PEEK test for SOCK_SEQPACKET. It works in the same way
as for SOCK_STREAM, except it also checks combination of MSG_TRUNC
and MSG_PEEK.
Head is:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d20dd0ea14072e8a90ff864b2c1603bd68920b4b
Arseniy Krasnov (4):
virtio/vsock: rework MSG_PEEK for SOCK_STREAM
virtio/vsock: support MSG_PEEK for SOCK_SEQPACKET
vsock/test: rework MSG_PEEK test for SOCK_STREAM
vsock/test: MSG_PEEK test for SOCK_SEQPACKET
net/vmw_vsock/virtio_transport_common.c | 104 +++++++++++++++-----
tools/testing/vsock/vsock_test.c | 124 ++++++++++++++++++++++--
2 files changed, 196 insertions(+), 32 deletions(-)
--
2.25.1
This reworks current implementation of MSG_PEEK logic:
1) Replaces 'skb_queue_walk_safe()' with 'skb_queue_walk()'. There is
no need in the first one, as there are no removes of skb in loop.
2) Removes nested while loop - MSG_PEEK logic could be implemented
without it: just iterate over skbs without removing it and copy
data from each until destination buffer is not full.
Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 41 ++++++++++++-------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index b769fc258931..2ee40574c339 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -348,37 +348,34 @@ virtio_transport_stream_do_peek(struct vsock_sock *vsk,
size_t len)
{
struct virtio_vsock_sock *vvs = vsk->trans;
- size_t bytes, total = 0, off;
- struct sk_buff *skb, *tmp;
- int err = -EFAULT;
+ struct sk_buff *skb;
+ size_t total = 0;
+ int err;
spin_lock_bh(&vvs->rx_lock);
- skb_queue_walk_safe(&vvs->rx_queue, skb, tmp) {
- off = 0;
+ skb_queue_walk(&vvs->rx_queue, skb) {
+ size_t bytes;
- if (total == len)
- break;
+ bytes = len - total;
+ if (bytes > skb->len)
+ bytes = skb->len;
- while (total < len && off < skb->len) {
- bytes = len - total;
- if (bytes > skb->len - off)
- bytes = skb->len - off;
+ spin_unlock_bh(&vvs->rx_lock);
- /* sk_lock is held by caller so no one else can dequeue.
- * Unlock rx_lock since memcpy_to_msg() may sleep.
- */
- spin_unlock_bh(&vvs->rx_lock);
+ /* sk_lock is held by caller so no one else can dequeue.
+ * Unlock rx_lock since memcpy_to_msg() may sleep.
+ */
+ err = memcpy_to_msg(msg, skb->data, bytes);
+ if (err)
+ goto out;
- err = memcpy_to_msg(msg, skb->data + off, bytes);
- if (err)
- goto out;
+ total += bytes;
- spin_lock_bh(&vvs->rx_lock);
+ spin_lock_bh(&vvs->rx_lock);
- total += bytes;
- off += bytes;
- }
+ if (total == len)
+ break;
}
spin_unlock_bh(&vvs->rx_lock);
--
2.25.1
This new version makes test more complicated by adding empty read,
partial read and data comparisons between MSG_PEEK and normal reads.
Signed-off-by: Arseniy Krasnov <[email protected]>
---
tools/testing/vsock/vsock_test.c | 66 ++++++++++++++++++++++++++++++--
1 file changed, 63 insertions(+), 3 deletions(-)
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index ac1bd3ac1533..104ac102e411 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -255,9 +255,13 @@ static void test_stream_multiconn_server(const struct test_opts *opts)
close(fds[i]);
}
+#define MSG_PEEK_BUF_LEN 64
+
static void test_stream_msg_peek_client(const struct test_opts *opts)
{
+ unsigned char buf[MSG_PEEK_BUF_LEN];
int fd;
+ int i;
fd = vsock_stream_connect(opts->peer_cid, 1234);
if (fd < 0) {
@@ -265,12 +269,21 @@ static void test_stream_msg_peek_client(const struct test_opts *opts)
exit(EXIT_FAILURE);
}
- send_byte(fd, 1, 0);
+ for (i = 0; i < sizeof(buf); i++)
+ buf[i] = rand() & 0xFF;
+
+ control_expectln("SRVREADY");
+
+ send(fd, buf, sizeof(buf), 0);
close(fd);
}
static void test_stream_msg_peek_server(const struct test_opts *opts)
{
+ unsigned char buf_half[MSG_PEEK_BUF_LEN / 2];
+ unsigned char buf_normal[MSG_PEEK_BUF_LEN];
+ unsigned char buf_peek[MSG_PEEK_BUF_LEN];
+ ssize_t res;
int fd;
fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL);
@@ -279,8 +292,55 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
exit(EXIT_FAILURE);
}
- recv_byte(fd, 1, MSG_PEEK);
- recv_byte(fd, 1, 0);
+ /* Peek from empty socket. */
+ res = recv(fd, buf_peek, sizeof(buf_peek), MSG_PEEK | MSG_DONTWAIT);
+ if (res != -1) {
+ fprintf(stderr, "expected recv(2) failure, got %zi\n", res);
+ exit(EXIT_FAILURE);
+ }
+
+ if (errno != EAGAIN) {
+ perror("EAGAIN expected");
+ exit(EXIT_FAILURE);
+ }
+
+ control_writeln("SRVREADY");
+
+ /* Peek part of data. */
+ res = recv(fd, buf_half, sizeof(buf_half), MSG_PEEK);
+ if (res != sizeof(buf_half)) {
+ fprintf(stderr, "recv(2) + MSG_PEEK, expected %zu, got %zi\n",
+ sizeof(buf_half), res);
+ exit(EXIT_FAILURE);
+ }
+
+ /* Peek whole data. */
+ res = recv(fd, buf_peek, sizeof(buf_peek), MSG_PEEK);
+ if (res != sizeof(buf_peek)) {
+ fprintf(stderr, "recv(2) + MSG_PEEK, expected %zu, got %zi\n",
+ sizeof(buf_peek), res);
+ exit(EXIT_FAILURE);
+ }
+
+ /* Compare partial and full peek. */
+ if (memcmp(buf_half, buf_peek, sizeof(buf_half))) {
+ fprintf(stderr, "Partial peek data mismatch\n");
+ exit(EXIT_FAILURE);
+ }
+
+ res = recv(fd, buf_normal, sizeof(buf_normal), 0);
+ if (res != sizeof(buf_normal)) {
+ fprintf(stderr, "recv(2), expected %zu, got %zi\n",
+ sizeof(buf_normal), res);
+ exit(EXIT_FAILURE);
+ }
+
+ /* Compare full peek and normal read. */
+ if (memcmp(buf_peek, buf_normal, sizeof(buf_peek))) {
+ fprintf(stderr, "Full peek data mismatch\n");
+ exit(EXIT_FAILURE);
+ }
+
close(fd);
}
--
2.25.1
On Sun, Jun 18, 2023 at 09:24:48AM +0300, Arseniy Krasnov wrote:
>This reworks current implementation of MSG_PEEK logic:
>1) Replaces 'skb_queue_walk_safe()' with 'skb_queue_walk()'. There is
> no need in the first one, as there are no removes of skb in loop.
>2) Removes nested while loop - MSG_PEEK logic could be implemented
> without it: just iterate over skbs without removing it and copy
> data from each until destination buffer is not full.
>
>Signed-off-by: Arseniy Krasnov <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 41 ++++++++++++-------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
Great clean up!
LGTM, but @Bobby can you also take a look?
Thanks,
Stefano
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index b769fc258931..2ee40574c339 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -348,37 +348,34 @@ virtio_transport_stream_do_peek(struct vsock_sock *vsk,
> size_t len)
> {
> struct virtio_vsock_sock *vvs = vsk->trans;
>- size_t bytes, total = 0, off;
>- struct sk_buff *skb, *tmp;
>- int err = -EFAULT;
>+ struct sk_buff *skb;
>+ size_t total = 0;
>+ int err;
>
> spin_lock_bh(&vvs->rx_lock);
>
>- skb_queue_walk_safe(&vvs->rx_queue, skb, tmp) {
>- off = 0;
>+ skb_queue_walk(&vvs->rx_queue, skb) {
>+ size_t bytes;
>
>- if (total == len)
>- break;
>+ bytes = len - total;
>+ if (bytes > skb->len)
>+ bytes = skb->len;
>
>- while (total < len && off < skb->len) {
>- bytes = len - total;
>- if (bytes > skb->len - off)
>- bytes = skb->len - off;
>+ spin_unlock_bh(&vvs->rx_lock);
>
>- /* sk_lock is held by caller so no one else can dequeue.
>- * Unlock rx_lock since memcpy_to_msg() may sleep.
>- */
>- spin_unlock_bh(&vvs->rx_lock);
>+ /* sk_lock is held by caller so no one else can dequeue.
>+ * Unlock rx_lock since memcpy_to_msg() may sleep.
>+ */
>+ err = memcpy_to_msg(msg, skb->data, bytes);
>+ if (err)
>+ goto out;
>
>- err = memcpy_to_msg(msg, skb->data + off, bytes);
>- if (err)
>- goto out;
>+ total += bytes;
>
>- spin_lock_bh(&vvs->rx_lock);
>+ spin_lock_bh(&vvs->rx_lock);
>
>- total += bytes;
>- off += bytes;
>- }
>+ if (total == len)
>+ break;
> }
>
> spin_unlock_bh(&vvs->rx_lock);
>--
>2.25.1
>
On Sun, Jun 18, 2023 at 09:24:47AM +0300, Arseniy Krasnov wrote:
>Hello,
>
>This patchset does several things around MSG_PEEK flag support. In
>general words it reworks MSG_PEEK test and adds support for this flag
>in SOCK_SEQPACKET logic. Here is per-patch description:
>
>1) This is cosmetic change for SOCK_STREAM implementation of MSG_PEEK:
> 1) I think there is no need of "safe" mode walk here as there is no
> "unlink" of skbs inside loop (it is MSG_PEEK mode - we don't change
> queue).
> 2) Nested while loop is removed: in case of MSG_PEEK we just walk
> over skbs and copy data from each one. I guess this nested loop
> even didn't behave as loop - it always executed just for single
> iteration.
>
>2) This adds MSG_PEEK support for SOCK_SEQPACKET. It could be implemented
> be reworking MSG_PEEK callback for SOCK_STREAM to support SOCK_SEQPACKET
> also, but I think it will be more simple and clear from potential
> bugs to implemented it as separate function thus not mixing logics
> for both types of socket. So I've added it as dedicated function.
>
>3) This is reworked MSG_PEEK test for SOCK_STREAM. Previous version just
> sent single byte, then tried to read it with MSG_PEEK flag, then read
> it in normal way. New version is more complex: now sender uses buffer
> instead of single byte and this buffer is initialized with random
> values. Receiver tests several things:
> 1) Read empty socket with MSG_PEEK flag.
> 2) Read part of buffer with MSG_PEEK flag.
> 3) Read whole buffer with MSG_PEEK flag, then checks that it is same
> as buffer from 2) (limited by size of buffer from 2) of course).
> 4) Read whole buffer without any flags, then checks that is is same
> as buffer from 3).
>
>4) This is MSG_PEEK test for SOCK_SEQPACKET. It works in the same way
> as for SOCK_STREAM, except it also checks combination of MSG_TRUNC
> and MSG_PEEK.
>
>Head is:
>https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d20dd0ea14072e8a90ff864b2c1603bd68920b4b
Nice cleanup, LGTM, but I'd like a comment from Bobby.
Thanks,
Stefano
On 26.06.2023 19:30, Stefano Garzarella wrote:
> On Sun, Jun 18, 2023 at 09:24:47AM +0300, Arseniy Krasnov wrote:
>> Hello,
>>
>> This patchset does several things around MSG_PEEK flag support. In
>> general words it reworks MSG_PEEK test and adds support for this flag
>> in SOCK_SEQPACKET logic. Here is per-patch description:
>>
>> 1) This is cosmetic change for SOCK_STREAM implementation of MSG_PEEK:
>> 1) I think there is no need of "safe" mode walk here as there is no
>> "unlink" of skbs inside loop (it is MSG_PEEK mode - we don't change
>> queue).
>> 2) Nested while loop is removed: in case of MSG_PEEK we just walk
>> over skbs and copy data from each one. I guess this nested loop
>> even didn't behave as loop - it always executed just for single
>> iteration.
>>
>> 2) This adds MSG_PEEK support for SOCK_SEQPACKET. It could be implemented
>> be reworking MSG_PEEK callback for SOCK_STREAM to support SOCK_SEQPACKET
>> also, but I think it will be more simple and clear from potential
>> bugs to implemented it as separate function thus not mixing logics
>> for both types of socket. So I've added it as dedicated function.
>>
>> 3) This is reworked MSG_PEEK test for SOCK_STREAM. Previous version just
>> sent single byte, then tried to read it with MSG_PEEK flag, then read
>> it in normal way. New version is more complex: now sender uses buffer
>> instead of single byte and this buffer is initialized with random
>> values. Receiver tests several things:
>> 1) Read empty socket with MSG_PEEK flag.
>> 2) Read part of buffer with MSG_PEEK flag.
>> 3) Read whole buffer with MSG_PEEK flag, then checks that it is same
>> as buffer from 2) (limited by size of buffer from 2) of course).
>> 4) Read whole buffer without any flags, then checks that is is same
>> as buffer from 3).
>>
>> 4) This is MSG_PEEK test for SOCK_SEQPACKET. It works in the same way
>> as for SOCK_STREAM, except it also checks combination of MSG_TRUNC
>> and MSG_PEEK.
>>
>> Head is:
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d20dd0ea14072e8a90ff864b2c1603bd68920b4b
>
> Nice cleanup, LGTM, but I'd like a comment from Bobby.
Got it, thanks!
Thanks, Arseniy
>
> Thanks,
> Stefano
>
On Sun, Jun 18, 2023 at 09:24:48AM +0300, Arseniy Krasnov wrote:
> This reworks current implementation of MSG_PEEK logic:
> 1) Replaces 'skb_queue_walk_safe()' with 'skb_queue_walk()'. There is
> no need in the first one, as there are no removes of skb in loop.
> 2) Removes nested while loop - MSG_PEEK logic could be implemented
> without it: just iterate over skbs without removing it and copy
> data from each until destination buffer is not full.
>
> Signed-off-by: Arseniy Krasnov <[email protected]>
> ---
> net/vmw_vsock/virtio_transport_common.c | 41 ++++++++++++-------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index b769fc258931..2ee40574c339 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -348,37 +348,34 @@ virtio_transport_stream_do_peek(struct vsock_sock *vsk,
> size_t len)
> {
> struct virtio_vsock_sock *vvs = vsk->trans;
> - size_t bytes, total = 0, off;
> - struct sk_buff *skb, *tmp;
> - int err = -EFAULT;
> + struct sk_buff *skb;
> + size_t total = 0;
> + int err;
>
> spin_lock_bh(&vvs->rx_lock);
>
> - skb_queue_walk_safe(&vvs->rx_queue, skb, tmp) {
> - off = 0;
> + skb_queue_walk(&vvs->rx_queue, skb) {
> + size_t bytes;
>
> - if (total == len)
> - break;
> + bytes = len - total;
> + if (bytes > skb->len)
> + bytes = skb->len;
>
> - while (total < len && off < skb->len) {
> - bytes = len - total;
> - if (bytes > skb->len - off)
> - bytes = skb->len - off;
> + spin_unlock_bh(&vvs->rx_lock);
>
> - /* sk_lock is held by caller so no one else can dequeue.
> - * Unlock rx_lock since memcpy_to_msg() may sleep.
> - */
> - spin_unlock_bh(&vvs->rx_lock);
> + /* sk_lock is held by caller so no one else can dequeue.
> + * Unlock rx_lock since memcpy_to_msg() may sleep.
> + */
> + err = memcpy_to_msg(msg, skb->data, bytes);
> + if (err)
> + goto out;
>
> - err = memcpy_to_msg(msg, skb->data + off, bytes);
> - if (err)
> - goto out;
> + total += bytes;
>
> - spin_lock_bh(&vvs->rx_lock);
> + spin_lock_bh(&vvs->rx_lock);
>
> - total += bytes;
> - off += bytes;
> - }
> + if (total == len)
> + break;
> }
>
> spin_unlock_bh(&vvs->rx_lock);
> --
> 2.25.1
>
That cleans up nicely!
LGTM.
Reviewed-by: Bobby Eshleman <[email protected]>