Re: [Qemu-devel] [PATCH v3 29/46] ivshmem: error on too many eventfd received

[Marc-André Lureau]

Hi

On Wed, Sep 16, 2015 at 2:14 PM, Claudio Fontana
<address@hidden> wrote:
> On 15.09.2015 18:07, address@hidden wrote:
>> From: Marc-André Lureau <address@hidden>
>>
>> The number of eventfd that can be handled per peer is limited by the
>> number of vectors. Return an error when receiving too many of them.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  hw/misc/ivshmem.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index b9c78cd..63e4c4f 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -569,6 +569,13 @@ static void ivshmem_read(void *opaque, const uint8_t 
>> *buf, int size)
>>      }
>>
>>      /* get a new eventfd */
>> +    if (peer->nb_eventfds >= s->vectors) {
>> +        error_report("Too many eventfd received, device has %d vectors",
>> +                     s->vectors);
>> +        close(incoming_fd);
>> +        return;
>> +    }
>> +
>>      nth_eventfd = peer->nb_eventfds++;
>>
>>      /* this is an eventfd for a particular peer VM */
>>
>
> can the device still operate if we detect these errors at ivshmem_read time?
>
> I am referring also to the other checks happening in ivshmem_read doing
>
> if ([something fails]) {
>     error_report("abcabc");
>     /* close(), ... */
>     return;
> }
>
> Can the device stop operating if these conditions happen?

Yes, it simply closes the "new" incoming_fd. So if the server sends
extra eventfd for peers that we can't handle, we won't be able to
notify those peers. But the rest of the peers and function is still
working.

This is btw, what the current code is doing (only the variable is
called tmp_fd before I removed it in "remove unnecessary dup()" patch)

> If so, do we have to put the device into a non-operating state, where all 
> read/writes are ignored? Should there be a ivshmem status flag for ERROR?
> Should we exit(1)?
>
> note: I don't know what the "proper" behavior should be, but I am concerned 
> about the runtime stability of the software which uses the device.

It's likely a misconfiguration. So having error reported seems like a
sane and enough thing to do.

-- 
Marc-André Lureau