2023-03-30 20:30:54

by Arseniy Krasnov

[permalink] [raw]
Subject: [RFC PATCH v3 0/4] vsock: return errors other than -ENOMEM to socket

Hello,

this patchset removes behaviour, where error code returned from any
transport was always switched to ENOMEM. This works in the same way as
patch from Bobby Eshleman:
commit c43170b7e157 ("vsock: return errors other than -ENOMEM to socket"),
but for receive calls. VMCI transport is also updated (both tx and rx
SOCK_STREAM callbacks), because it returns VMCI specific error code to
af_vsock.c (like VMCI_ERROR_*). At the same time, virtio and Hyper-V
transports are using general error codes, so there is no need to update
them.

vsock_test suite is also updated.

Link to v1:
https://lore.kernel.org/netdev/[email protected]/
Link to v2:
https://lore.kernel.org/netdev/[email protected]/

Changelog:

v1 -> v2:
- Add patch for VMCI as Vishnu Dasa suggested.
v2 -> v3:
- Change type of 'err' var in VMCI patches from 'int' to 'ssize_t'.
- Split VMCI patch to two patches: for send and for receive cases.
- Reorder patches: move VMCI before af_vsock.c.

Arseniy Krasnov (4):
vsock/vmci: convert VMCI error code to -ENOMEM on send
vsock/vmci: convert VMCI error code to -ENOMEM on receive
vsock: return errors other than -ENOMEM to socket
vsock/test: update expected return values

net/vmw_vsock/af_vsock.c | 4 ++--
net/vmw_vsock/vmci_transport.c | 19 ++++++++++++++++---
tools/testing/vsock/vsock_test.c | 4 ++--
3 files changed, 20 insertions(+), 7 deletions(-)

--
2.25.1


2023-03-30 20:32:15

by Arseniy Krasnov

[permalink] [raw]
Subject: [RFC PATCH v3 1/4] vsock/vmci: convert VMCI error code to -ENOMEM on send

This adds conversion of VMCI specific error code to general -ENOMEM. It
is needed, because af_vsock.c passes error value returned from transport
to the user, which does not expect to get VMCI_ERROR_* values.

Fixes: c43170b7e157 ("vsock: return errors other than -ENOMEM to socket")
Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/vmci_transport.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 36eb16a40745..95cc4d79ba29 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1842,7 +1842,13 @@ static ssize_t vmci_transport_stream_enqueue(
struct msghdr *msg,
size_t len)
{
- return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+ ssize_t err;
+
+ err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+ if (err < 0)
+ err = -ENOMEM;
+
+ return err;
}

static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
--
2.25.1

2023-03-30 20:32:28

by Arseniy Krasnov

[permalink] [raw]
Subject: [RFC PATCH v3 2/4] vsock/vmci: convert VMCI error code to -ENOMEM on receive

This adds conversion of VMCI specific error code to general -ENOMEM. It
is needed, because af_vsock.c passes error value returned from transport
to the user, which does not expect to get VMCI_ERROR_* values.

Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/vmci_transport.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 95cc4d79ba29..b370070194fa 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1831,10 +1831,17 @@ static ssize_t vmci_transport_stream_dequeue(
size_t len,
int flags)
{
+ ssize_t err;
+
if (flags & MSG_PEEK)
- return vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
+ err = vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
else
- return vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
+ err = vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
+
+ if (err < 0)
+ err = -ENOMEM;
+
+ return err;
}

static ssize_t vmci_transport_stream_enqueue(
--
2.25.1

2023-03-30 20:33:03

by Arseniy Krasnov

[permalink] [raw]
Subject: [RFC PATCH v3 3/4] vsock: return errors other than -ENOMEM to socket

This removes behaviour, where error code returned from any transport
was always switched to ENOMEM. This works in the same way as:
commit
c43170b7e157 ("vsock: return errors other than -ENOMEM to socket"),
but for receive calls.

Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/af_vsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 5f2dda35c980..413407bb646c 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2043,7 +2043,7 @@ static int __vsock_stream_recvmsg(struct sock *sk, struct msghdr *msg,

read = transport->stream_dequeue(vsk, msg, len - copied, flags);
if (read < 0) {
- err = -ENOMEM;
+ err = read;
break;
}

@@ -2094,7 +2094,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
msg_len = transport->seqpacket_dequeue(vsk, msg, flags);

if (msg_len < 0) {
- err = -ENOMEM;
+ err = msg_len;
goto out;
}

--
2.25.1

2023-03-30 20:33:37

by Arseniy Krasnov

[permalink] [raw]
Subject: [RFC PATCH v3 4/4] vsock/test: update expected return values

This updates expected return values for invalid buffer test. Now such
values are returned from transport, not from af_vsock.c.

Signed-off-by: Arseniy Krasnov <[email protected]>
Reviewed-by: Stefano Garzarella <[email protected]>
---
tools/testing/vsock/vsock_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 3de10dbb50f5..a91d0ef963be 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -723,7 +723,7 @@ static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opt
exit(EXIT_FAILURE);
}

- if (errno != ENOMEM) {
+ if (errno != EFAULT) {
perror("unexpected errno of 'broken_buf'");
exit(EXIT_FAILURE);
}
@@ -887,7 +887,7 @@ static void test_inv_buf_client(const struct test_opts *opts, bool stream)
exit(EXIT_FAILURE);
}

- if (errno != ENOMEM) {
+ if (errno != EFAULT) {
fprintf(stderr, "unexpected recv(2) errno %d\n", errno);
exit(EXIT_FAILURE);
}
--
2.25.1

2023-03-30 20:34:36

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RFC PATCH v3 2/4] vsock/vmci: convert VMCI error code to -ENOMEM on receive



On 30.03.2023 23:13, Arseniy Krasnov wrote:
> This adds conversion of VMCI specific error code to general -ENOMEM. It
> is needed, because af_vsock.c passes error value returned from transport
> to the user, which does not expect to get VMCI_ERROR_* values.

@Stefano, I have some doubts about this commit message, as it says "... af_vsock.c
passes error value returned from transport to the user ...", but this
behaviour is implemented only in the next patch. Is it ok, if both patches
are in a single patchset?

For patch 1 I think it is ok, as it fixes current implementation.

Thanks, Arseniy

>
> Signed-off-by: Arseniy Krasnov <[email protected]>
> ---
> net/vmw_vsock/vmci_transport.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 95cc4d79ba29..b370070194fa 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1831,10 +1831,17 @@ static ssize_t vmci_transport_stream_dequeue(
> size_t len,
> int flags)
> {
> + ssize_t err;
> +
> if (flags & MSG_PEEK)
> - return vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
> + err = vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
> else
> - return vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
> + err = vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
> +
> + if (err < 0)
> + err = -ENOMEM;
> +
> + return err;
> }
>
> static ssize_t vmci_transport_stream_enqueue(

2023-03-30 21:53:25

by Vishnu Dasa

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] vsock/vmci: convert VMCI error code to -ENOMEM on send



> On Mar 30, 2023, at 1:12 PM, Arseniy Krasnov <[email protected]> wrote:
>
> !! External Email
>
> This adds conversion of VMCI specific error code to general -ENOMEM. It
> is needed, because af_vsock.c passes error value returned from transport
> to the user, which does not expect to get VMCI_ERROR_* values.
>
> Fixes: c43170b7e157 ("vsock: return errors other than -ENOMEM to socket")
> Signed-off-by: Arseniy Krasnov <[email protected]>

Thanks, looks good to me.

Reviewed-by: Vishnu Dasa <[email protected]>

> ---
> net/vmw_vsock/vmci_transport.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 36eb16a40745..95cc4d79ba29 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1842,7 +1842,13 @@ static ssize_t vmci_transport_stream_enqueue(
> struct msghdr *msg,
> size_t len)
> {
> - return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> + ssize_t err;
> +
> + err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> + if (err < 0)
> + err = -ENOMEM;
> +
> + return err;
> }
>
> static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
> --
> 2.25.1
>
> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.

2023-03-30 21:55:03

by Vishnu Dasa

[permalink] [raw]
Subject: Re: [RFC PATCH v3 2/4] vsock/vmci: convert VMCI error code to -ENOMEM on receive



> On Mar 30, 2023, at 1:18 PM, Arseniy Krasnov <[email protected]> wrote:
>
> !! External Email
>
> On 30.03.2023 23:13, Arseniy Krasnov wrote:
>> This adds conversion of VMCI specific error code to general -ENOMEM. It
>> is needed, because af_vsock.c passes error value returned from transport
>> to the user, which does not expect to get VMCI_ERROR_* values.
>
> @Stefano, I have some doubts about this commit message, as it says "... af_vsock.c
> passes error value returned from transport to the user ...", but this
> behaviour is implemented only in the next patch. Is it ok, if both patches
> are in a single patchset?
>
> For patch 1 I think it is ok, as it fixes current implementation.
>
> Thanks, Arseniy
>
>>
>> Signed-off-by: Arseniy Krasnov <[email protected]>

Code change looks good to me. Will let you figure out the commit message
with Stefano. Thanks!

Reviewed-by: Vishnu Dasa <[email protected]>

>> ---
>> net/vmw_vsock/vmci_transport.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>> index 95cc4d79ba29..b370070194fa 100644
>> --- a/net/vmw_vsock/vmci_transport.c
>> +++ b/net/vmw_vsock/vmci_transport.c
>> @@ -1831,10 +1831,17 @@ static ssize_t vmci_transport_stream_dequeue(
>> size_t len,
>> int flags)
>> {
>> + ssize_t err;
>> +
>> if (flags & MSG_PEEK)
>> - return vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
>> + err = vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
>> else
>> - return vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
>> + err = vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +
>> + if (err < 0)
>> + err = -ENOMEM;
>> +
>> + return err;
>> }
>>
>> static ssize_t vmci_transport_stream_enqueue(
>
> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.

2023-03-31 07:15:40

by Stefano Garzarella

[permalink] [raw]
Subject: Re: [RFC PATCH v3 2/4] vsock/vmci: convert VMCI error code to -ENOMEM on receive

On Thu, Mar 30, 2023 at 11:18:36PM +0300, Arseniy Krasnov wrote:
>
>
>On 30.03.2023 23:13, Arseniy Krasnov wrote:
>> This adds conversion of VMCI specific error code to general -ENOMEM. It
>> is needed, because af_vsock.c passes error value returned from transport
>> to the user, which does not expect to get VMCI_ERROR_* values.
>
>@Stefano, I have some doubts about this commit message, as it says "... af_vsock.c
>passes error value returned from transport to the user ...", but this
>behaviour is implemented only in the next patch. Is it ok, if both patches
>are in a single patchset?

Yes indeed it is not clear. In my opinion we can do one of these 2
things:

1. Update the message, where we can say that this is a preparation patch
for the next changes where af_vsock.c will directly return transport
values to the user, so we need to return an errno.

2. Merge this patch and patch 3 in a single patch.

Both are fine for my point of view, take your choice ;-)

Thanks,
Stefano

2023-03-31 07:22:11

by Stefano Garzarella

[permalink] [raw]
Subject: Re: [RFC PATCH v3 1/4] vsock/vmci: convert VMCI error code to -ENOMEM on send

On Thu, Mar 30, 2023 at 11:12:44PM +0300, Arseniy Krasnov wrote:
>This adds conversion of VMCI specific error code to general -ENOMEM. It
>is needed, because af_vsock.c passes error value returned from transport
>to the user, which does not expect to get VMCI_ERROR_* values.
>
>Fixes: c43170b7e157 ("vsock: return errors other than -ENOMEM to socket")
>Signed-off-by: Arseniy Krasnov <[email protected]>
>---
> net/vmw_vsock/vmci_transport.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)

The patch LGTM, but I suggest to extract this patch from the series and
send it directly to the net tree, while other patches can be sent to
net-next.

Reviewed-by: Stefano Garzarella <[email protected]>

2023-03-31 08:08:48

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [RFC PATCH v3 2/4] vsock/vmci: convert VMCI error code to -ENOMEM on receive



On 31.03.2023 10:12, Stefano Garzarella wrote:
> On Thu, Mar 30, 2023 at 11:18:36PM +0300, Arseniy Krasnov wrote:
>>
>>
>> On 30.03.2023 23:13, Arseniy Krasnov wrote:
>>> This adds conversion of VMCI specific error code to general -ENOMEM. It
>>> is needed, because af_vsock.c passes error value returned from transport
>>> to the user, which does not expect to get VMCI_ERROR_* values.
>>
>> @Stefano, I have some doubts about this commit message, as it says "... af_vsock.c
>> passes error value returned from transport to the user ...", but this
>> behaviour is implemented only in the next patch. Is it ok, if both patches
>> are in a single patchset?
>
> Yes indeed it is not clear. In my opinion we can do one of these 2
> things:
>
> 1. Update the message, where we can say that this is a preparation patch
>    for the next changes where af_vsock.c will directly return transport
>    values to the user, so we need to return an errno.
>
> 2. Merge this patch and patch 3 in a single patch.
>
> Both are fine for my point of view, take your choice ;-)

Ok! Thanks for this!

Thanks, Arseniy

>
> Thanks,
> Stefano
>