Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full

[Didier Pallard <didier.pallard@6wind.com>]

On 12/10/2015 01:56 PM, Victor Kaplansky wrote:
> On Wed, Dec 09, 2015 at 06:06:06PM +0100, Didier Pallard wrote:
>> On 12/09/2015 04:59 PM, Victor Kaplansky wrote:
>>> On Mon, Dec 07, 2015 at 02:31:36PM +0100, Marc-André Lureau wrote:
>>>> Hi
>>>>
>>>> On Thu, Dec 3, 2015 at 10:53 AM, Didier Pallard
>>>> <didier.pallard@6wind.com> wrote:
>>>>> unix_send_msgfds is used by vhost-user control socket. qemu_chr_fe_write_all
>>>>> is used to send a message and retries as long as EAGAIN errno is set,
>>>>> but write_msgfds buffer is freed after first EAGAIN failure, causing
>>>>> message to be sent without proper fds attachment.
>>>>>
>>>>> In case unix_send_msgfds is called through qemu_chr_fe_write, it will be
>>>>> user responsability to resend message as is or to free write_msgfds
>>>>> using set_msgfds(0)
>>>>>
>>>>> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
>>>>> Reviewed-by: Thibaut Collet <thibaut.collet@6wind.com>
>>>>> ---
>>>>>   qemu-char.c | 10 ++++++++++
>>>>>   1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/qemu-char.c b/qemu-char.c
>>>>> index 5448b0f..26d5f2e 100644
>>>>> --- a/qemu-char.c
>>>>> +++ b/qemu-char.c
>>>>> @@ -2614,6 +2614,16 @@ static int unix_send_msgfds(CharDriverState *chr, const uint8_t *buf, int len)
>>>>>           r = sendmsg(s->fd, &msgh, 0);
>>>>>       } while (r < 0 && errno == EINTR);
>>>>>
>>>>> +    /* Ancillary data are not sent if no byte is written
>>>>> +     * so don't free msgfds buffer if return value is EAGAIN
>>>>> +     * If called from qemu_chr_fe_write_all retry will come soon
>>>>> +     * If called from qemu_chr_fe_write, it is the user responsibility
>>>>> +     * to resend message or free fds using set_msgfds(0)
>>>>> +     */
>>>>> +    if (r < 0 && errno == EAGAIN) {
>>>>> +        return r;
>>>>> +    }
>>>>> +
>>>>
>>>> This looks reasonable to me. However, I don't know what happens with
>>>> partial write of ancillary data. Hopefully it's all or nothing.
>>>> Apparently, reading unix_stream_sendmsg() in kernel shows that as long
>>>> as a few bytes have been sent, the ancillary data is sent. So it looks
>>>> like it still does the right thing in case of a partial write.
>>>
>>> If I may put my two cents in, it looks to me very similar to an
>>> fd leakage on back-end side. When a new set_call_fd request
>>> arrives, it is very easy to forget closing the previous file
>>> descriptor. As result, if interrupts are actively maksed/unmasked
>>> by the guest, the back-end can easily reach maximum fds, which
>>> will cause receiving side silently drop new fds in aux data.
>>> --Victor
>>>
>>
>> Hi victor,
>> This is not a problem of fd exausted. This was my first axe of
>> investigation, but fd management is correct in our vhost-user backend, there
>> is no fd leakage.
>
> That's good.
>
>> And i guess you are refering to the problem fixed by patches 2 and 3, since
>> the problem corrected by this patch is a message arriving from qemu without
>> ancillary data, whatever the state of the fds in the vhost-user backend.
>
> I'm talking about the problem that supposed to be fixed by the
> first patch. It is not clear to me how the patch fixes the
> partial send. sendmsg() is called in qemu-char.c:unix_send_msgfds
> with zero flags, which means a blocking operation, so I'm
> surprised that sendmsg can return with errno == EAGAIN.
>

Well, vhost-user socket is started with following chardev:
-chardev socket,id=vhostuserchr0,path=/tmp/vhost_sock0,server
and according to code in tcp_chr_add_client:
static int tcp_chr_add_client(CharDriverState *chr, int fd)
{
...
     qemu_set_nonblock(fd);

So fd is set in non blocking mode. This is enough to have an
EAGAIN returned value on socket buffer full, whatever flags used in 
sendmsg, i think.
Perhaps changing the blocking mode here may also correct the first 
problem, but I am not able to measure the impact that may have such a 
modification...


>>
>> thanks
>> didier
>>
>>>>
>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>>>       /* free the written msgfds, no matter what */
>>>>>       if (s->write_msgfds_num) {
>>>>>           g_free(s->write_msgfds);
>>>>> --
>>>>> 2.1.4
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Marc-André Lureau
>>>>
>>
>>
>>