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

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

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

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