Re: [PATCH v2 2/3] virtio-balloon: Add locking to prevent possible race when starting hinting

[David Hildenbrand]

On 06.07.20 23:14, Alexander Duyck wrote:
> From: Alexander Duyck 
> 
> There is already locking in place when we are stopping free page hinting
> but there is not similar protections in place when we start. I can only
> assume this was overlooked as in most cases the page hinting should not be
> occurring when we are starting the hinting, however there is still a chance
> we could be processing hints by the time we get back around to restarting
> the hinting so we are better off making sure to protect the state with the
> mutex lock rather than just updating the value with no protections.
> 
> Signed-off-by: Alexander Duyck 
> ---
>  hw/virtio/virtio-balloon.c |4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 0c0fd7114799..b3e96a822b4d 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -593,6 +593,8 @@ static void virtio_balloon_free_page_start(VirtIOBalloon 
> *s)
>  return;
>  }
>  
> +qemu_mutex_lock(>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;
> @@ -601,6 +603,8 @@ static void virtio_balloon_free_page_start(VirtIOBalloon 
> *s)
>  }
>  
>  s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> +qemu_mutex_unlock(>free_page_lock);
> +
>  virtio_notify_config(vdev);
>  }
>  
> 

Yes, makes sense, thanks

Acked-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] virtio-balloon: Add locking to prevent possible race when starting hinting

[Peter Maydell]

On Thu, 9 Jul 2020 at 16:23, Peter Maydell  wrote:

Side note: the virtio-dev mailing list produces an error
including a random mysqldb related failure if you try to post
to it as a non-list-member, which makes it awkward to cross-post
to it and to a public list like qemu-devel:

: permission denied. Command output: Traceback
(most recent call last):   File "/home/oasis/bin/kmlm_crosspost_issubn",
line 14, in  import MySQLdb ImportError: No module named
MySQLdb Sorry, only subscribers may post. If you are a subscriber, please
forward this message to virtio-dev-ow...@lists.oasis-open.org to get your
new address included (#5.7.2) ERROR: postqmail-local Error #77

thanks
-- PMM

Re: [PATCH v2 2/3] virtio-balloon: Add locking to prevent possible race when starting hinting

[Peter Maydell]

On Mon, 6 Jul 2020 at 22:15, Alexander Duyck  wrote:
>
> From: Alexander Duyck 
>
> There is already locking in place when we are stopping free page hinting
> but there is not similar protections in place when we start. I can only
> assume this was overlooked as in most cases the page hinting should not be
> occurring when we are starting the hinting, however there is still a chance
> we could be processing hints by the time we get back around to restarting
> the hinting so we are better off making sure to protect the state with the
> mutex lock rather than just updating the value with no protections.
>
> Signed-off-by: Alexander Duyck 
> ---
>  hw/virtio/virtio-balloon.c |4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 0c0fd7114799..b3e96a822b4d 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -593,6 +593,8 @@ static void virtio_balloon_free_page_start(VirtIOBalloon 
> *s)
>  return;
>  }
>
> +qemu_mutex_lock(>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;
> @@ -601,6 +603,8 @@ static void virtio_balloon_free_page_start(VirtIOBalloon 
> *s)
>  }
>
>  s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> +qemu_mutex_unlock(>free_page_lock);
> +
>  virtio_notify_config(vdev);
>  }

Coverity spotted this lack of locking too: CID 1430269.

thanks
-- PMM