diff mbox series

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

Message ID 20200706211407.20018.51061.stgit@localhost.localdomain (mailing list archive)
State New, archived
Headers show
Series virtio-balloon: Free page hinting clean-ups | expand

Commit Message

Alexander Duyck July 6, 2020, 9:14 p.m. UTC
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

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 <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c |    4 ++++
 1 file changed, 4 insertions(+)

Comments

Peter Maydell July 9, 2020, 3:23 p.m. UTC | #1
On Mon, 6 Jul 2020 at 22:15, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>
> 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 <alexander.h.duyck@linux.intel.com>
> ---
>  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(&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;
> @@ -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(&s->free_page_lock);
> +
>      virtio_notify_config(vdev);
>  }

Coverity spotted this lack of locking too: CID 1430269.

thanks
-- PMM
Peter Maydell July 9, 2020, 3:28 p.m. UTC | #2
On Thu, 9 Jul 2020 at 16:23, Peter Maydell <peter.maydell@linaro.org> 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:

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

thanks
-- PMM
David Hildenbrand July 10, 2020, 12:14 p.m. UTC | #3
On 06.07.20 23:14, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> 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 <alexander.h.duyck@linux.intel.com>
> ---
>  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(&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;
> @@ -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(&s->free_page_lock);
> +
>      virtio_notify_config(vdev);
>  }
>  
> 

Yes, makes sense, thanks

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

Patch

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(&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;
@@ -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(&s->free_page_lock);
+
     virtio_notify_config(vdev);
 }