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

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

[Arseniy Krasnov]

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/97f19214-ba04-c47e-7486-72e8aa16c690@sberdevices.ru/
Link to v2:
https://lore.kernel.org/netdev/60abc0da-0412-6e25-eeb0-8e32e3ec21e7@sberdevices.ru/

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

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

[Arseniy Krasnov]

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 <AVKrasnov@sberdevices.ru>
---
 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

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

[Arseniy Krasnov]

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 <AVKrasnov@sberdevices.ru>
---
 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

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

[Arseniy Krasnov]

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 <AVKrasnov@sberdevices.ru>
---
 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

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

[Arseniy Krasnov]

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 <AVKrasnov@sberdevices.ru>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 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

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

[Arseniy Krasnov]

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 <AVKrasnov@sberdevices.ru>
> ---
>  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(

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

[Vishnu Dasa]

> On Mar 30, 2023, at 1:12 PM, Arseniy Krasnov <AVKrasnov@sberdevices.ru> 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 <AVKrasnov@sberdevices.ru>

Thanks, looks good to me.

Reviewed-by: Vishnu Dasa <vdasa@vmware.com>

> ---
> 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.

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

[Vishnu Dasa]

> On Mar 30, 2023, at 1:18 PM, Arseniy Krasnov <AVKrasnov@sberdevices.ru> 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 <AVKrasnov@sberdevices.ru>

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

Reviewed-by: Vishnu Dasa <vdasa@vmware.com>

>> ---
>> 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.

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

[Stefano Garzarella]

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

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

[Stefano Garzarella]

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 <AVKrasnov@sberdevices.ru>
>---
> 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 <sgarzare@redhat.com>

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

[Arseniy Krasnov]

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
>