[v2] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)

[PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

Posted by Stefano Garzarella 1 year, 10 months ago

In vu_message_write() we use sendmsg() to send the message header,
then a write() to send the payload.

If sendmsg() fails we should avoid sending the payload, since we
were unable to send the header.

Discovered before fixing the issue with the previous patch, where
sendmsg() failed on macOS due to wrong parameters, but the frontend
still sent the payload which the backend incorrectly interpreted
as a wrong header.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 22bea0c775..a11afd1960 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
         rc = sendmsg(conn_fd, &msg, 0);
     } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
 
+    if (rc <= 0) {
+        vu_panic(dev, "Error while writing: %s", strerror(errno));
+        return false;
+    }
+
     if (vmsg->size) {
         do {
             if (vmsg->data) {
-- 
2.44.0

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

Posted by Eric Blake 1 year, 10 months ago

On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
> In vu_message_write() we use sendmsg() to send the message header,
> then a write() to send the payload.
> 
> If sendmsg() fails we should avoid sending the payload, since we
> were unable to send the header.
> 
> Discovered before fixing the issue with the previous patch, where
> sendmsg() failed on macOS due to wrong parameters, but the frontend
> still sent the payload which the backend incorrectly interpreted
> as a wrong header.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 22bea0c775..a11afd1960 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>          rc = sendmsg(conn_fd, &msg, 0);
>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>  
> +    if (rc <= 0) {

Is rejecting a 0 return value correct?  Technically, a 0 return means
a successful write of 0 bytes - but then again, it is unwise to
attempt to send an fd or other auxilliary ddata without at least one
regular byte, so we should not be attempting a write of 0 bytes.  So I
guess this one is okay, although I might have used < instead of <=.

> +        vu_panic(dev, "Error while writing: %s", strerror(errno));
> +        return false;
> +    }

At any rate, noticing the error is the correct thing to do.

> +
>      if (vmsg->size) {
>          do {
>              if (vmsg->data) {
> -- 
> 2.44.0
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

Posted by David Hildenbrand 1 year, 10 months ago

On 26.03.24 15:34, Eric Blake wrote:
> On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
>> In vu_message_write() we use sendmsg() to send the message header,
>> then a write() to send the payload.
>>
>> If sendmsg() fails we should avoid sending the payload, since we
>> were unable to send the header.
>>
>> Discovered before fixing the issue with the previous patch, where
>> sendmsg() failed on macOS due to wrong parameters, but the frontend
>> still sent the payload which the backend incorrectly interpreted
>> as a wrong header.
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>   subprojects/libvhost-user/libvhost-user.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
>> index 22bea0c775..a11afd1960 100644
>> --- a/subprojects/libvhost-user/libvhost-user.c
>> +++ b/subprojects/libvhost-user/libvhost-user.c
>> @@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>>           rc = sendmsg(conn_fd, &msg, 0);
>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>   
>> +    if (rc <= 0) {
> 
> Is rejecting a 0 return value correct?  Technically, a 0 return means
> a successful write of 0 bytes - but then again, it is unwise to
> attempt to send an fd or other auxilliary ddata without at least one
> regular byte, so we should not be attempting a write of 0 bytes.  So I
> guess this one is okay, although I might have used < instead of <=.

I was wondering if we could see some partial sendmsg()/write succeeding. 
Meaning, we transferred some bytes but not all, and we'd actually need 
to loop ...

-- 
Cheers,

David / dhildenb

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

Posted by Stefano Garzarella 1 year, 10 months ago

On Tue, Mar 26, 2024 at 03:36:52PM +0100, David Hildenbrand wrote:
>On 26.03.24 15:34, Eric Blake wrote:
>>On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
>>>In vu_message_write() we use sendmsg() to send the message header,
>>>then a write() to send the payload.
>>>
>>>If sendmsg() fails we should avoid sending the payload, since we
>>>were unable to send the header.
>>>
>>>Discovered before fixing the issue with the previous patch, where
>>>sendmsg() failed on macOS due to wrong parameters, but the frontend
>>>still sent the payload which the backend incorrectly interpreted
>>>as a wrong header.
>>>
>>>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>>---
>>>  subprojects/libvhost-user/libvhost-user.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>>diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
>>>index 22bea0c775..a11afd1960 100644
>>>--- a/subprojects/libvhost-user/libvhost-user.c
>>>+++ b/subprojects/libvhost-user/libvhost-user.c
>>>@@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>>>          rc = sendmsg(conn_fd, &msg, 0);
>>>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>>+    if (rc <= 0) {
>>
>>Is rejecting a 0 return value correct?  Technically, a 0 return means
>>a successful write of 0 bytes - but then again, it is unwise to
>>attempt to send an fd or other auxilliary ddata without at least one
>>regular byte, so we should not be attempting a write of 0 bytes.  So I
>>guess this one is okay, although I might have used < instead of <=.

I blindly copied what they already do at the end of the same function.
However, your observation is correct. That said they have a sendmsg()
to send the header. After this we have a write() loop to send the
payload.

Now if sendmsg() returns 0, but we have not sent all the header, what
should we do? Try sendmsg() again? For how many times?

>
>I was wondering if we could see some partial sendmsg()/write 
>succeeding. Meaning, we transferred some bytes but not all, and we'd 
>actually need to loop ...

Yep, true, but I would fix it in another patch/series if you agree.

Thanks,
Stefano

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

Posted by David Hildenbrand 1 year, 10 months ago

>>
>> I was wondering if we could see some partial sendmsg()/write
>> succeeding. Meaning, we transferred some bytes but not all, and we'd
>> actually need to loop ...
> 
> Yep, true, but I would fix it in another patch/series if you agree.

Absolutely.

-- 
Cheers,

David / dhildenb