Re: [PATCH 1/2] virtio-balloon: Prevent guest from starting a report when we didn't request one

[David Hildenbrand <david@redhat.com>]

>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -527,7 +527,8 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>>>              ret = false;
>>>              goto out;
>>>          }
>>> -        if (id == dev->free_page_report_cmd_id) {
>>> +        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
>>> +            id == dev->free_page_report_cmd_id) {
>>>              dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>>>          } else {
>>>              /*
>>
>> But doesn't that mean that, after the first hint, all further ones will
>> be discarded and we'll enter the STOP state in the else case? Or am I
>> missing something?
>>
>> Shouldn't this be something like
>>
>> if (id == dev->free_page_report_cmd_id) {
>>     if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
>>         dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>>     }
>>     /* Stay in FREE_PAGE_REPORT_S_START as long as the cmd_id match .*/
>> } else { ...
> 
> There should only be one element containing an outbuf at the start of
> the report. Once that is processed we should not see the driver
> sending additional outbufs unless it is sending the STOP command ID.

Ok, I assume what Linux guests do is considered the correct protocol.

[...]

> 
>>> @@ -592,14 +593,16 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>>>          return;
>>>      }
>>>
>>> -    if (s->free_page_report_cmd_id == UINT_MAX) {
>>> +    qemu_mutex_lock(&s->free_page_lock);
>>> +
>>> +    if (s->free_page_report_cmd_id++ == UINT_MAX) {
>>>          s->free_page_report_cmd_id =
>>>                         VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>> -    } else {
>>> -        s->free_page_report_cmd_id++;
>>>      }
>>
>> Somewhat unrelated cleanup.
> 
> Agreed. I can drop it if preferred. I just took care of it because I
> was adding the lock above and below to prevent us from getting into
> any wierd states where the command ID might be updated but the report
> status was not.

No hard feelings, it just makes reviewing harder, because one has to
investigate how the changes relate to the locking changes - to find out
they don't. :)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb