[Qemu-devel] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[Qemu-devel] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[Jeff Cody]

Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
checks for when qemu_mutex() functions are called without the
corresponding qemu_mutex_init() having initialized the mutex.

This uncovered a latent bug in qemu's nfs driver - in
nfs_client_close(), the NFSClient structure is overwritten with zeros,
prior to the mutex being destroyed.

Go ahead and destroy the mutex in nfs_client_close(), and change where
we call qemu_mutex_init() so that it is correctly balanced.

There are also a couple of memory leaks obscured by the memset, so this
fixes those as well.

Finally, we should be able to get rid of the memset(), as it isn't
necessary.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/nfs.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index d8db419..bec16b7 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client)
     if (client->context) {
         if (client->fh) {
             nfs_close(client->context, client->fh);
+            client->fh = NULL;
         }
         aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
                            false, NULL, NULL, NULL, NULL);
         nfs_destroy_context(client->context);
+        client->context = NULL;
     }
-    memset(client, 0, sizeof(NFSClient));
-}
-
-static void nfs_file_close(BlockDriverState *bs)
-{
-    NFSClient *client = bs->opaque;
-    nfs_client_close(client);
+    g_free(client->path);
     qemu_mutex_destroy(&client->mutex);
+    qapi_free_NFSServer(client->server);
+    client->server = NULL;
+}
+
+static void nfs_file_close(BlockDriverState *bs)
+{
+    NFSClient *client = bs->opaque;
+    nfs_client_close(client);
 }
 
 static NFSServer *nfs_config(QDict *options, Error **errp)
@@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
     struct stat st;
     char *file = NULL, *strp = NULL;
 
+    qemu_mutex_init(&client->mutex);
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
@@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
     if (ret < 0) {
         return ret;
     }
-    qemu_mutex_init(&client->mutex);
+
     bs->total_sectors = ret;
     ret = 0;
     return ret;
-- 
2.9.4

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[John Snow]

On 08/07/2017 06:29 PM, Jeff Cody wrote:
> Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
> checks for when qemu_mutex() functions are called without the
> corresponding qemu_mutex_init() having initialized the mutex.
> 
> This uncovered a latent bug in qemu's nfs driver - in
> nfs_client_close(), the NFSClient structure is overwritten with zeros,
> prior to the mutex being destroyed.
> 
> Go ahead and destroy the mutex in nfs_client_close(), and change where
> we call qemu_mutex_init() so that it is correctly balanced.
> 
> There are also a couple of memory leaks obscured by the memset, so this
> fixes those as well.
> 
> Finally, we should be able to get rid of the memset(), as it isn't
> necessary.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>


Looks sane to me.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

On Mon, Aug 07, 2017 at 06:29:09PM -0400, Jeff Cody wrote:
> Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
> checks for when qemu_mutex() functions are called without the
> corresponding qemu_mutex_init() having initialized the mutex.
> 
> This uncovered a latent bug in qemu's nfs driver - in
> nfs_client_close(), the NFSClient structure is overwritten with zeros,
> prior to the mutex being destroyed.
> 
> Go ahead and destroy the mutex in nfs_client_close(), and change where
> we call qemu_mutex_init() so that it is correctly balanced.
> 
> There are also a couple of memory leaks obscured by the memset, so this
> fixes those as well.
> 
> Finally, we should be able to get rid of the memset(), as it isn't
> necessary.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/nfs.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[Peter Lieven]

Am 08.08.2017 um 00:29 schrieb Jeff Cody:
> Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
> checks for when qemu_mutex() functions are called without the
> corresponding qemu_mutex_init() having initialized the mutex.
>
> This uncovered a latent bug in qemu's nfs driver - in
> nfs_client_close(), the NFSClient structure is overwritten with zeros,
> prior to the mutex being destroyed.
>
> Go ahead and destroy the mutex in nfs_client_close(), and change where
> we call qemu_mutex_init() so that it is correctly balanced.
>
> There are also a couple of memory leaks obscured by the memset, so this
> fixes those as well.
>
> Finally, we should be able to get rid of the memset(), as it isn't
> necessary.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/nfs.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/block/nfs.c b/block/nfs.c
> index d8db419..bec16b7 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client)
>       if (client->context) {
>           if (client->fh) {
>               nfs_close(client->context, client->fh);
> +            client->fh = NULL;
>           }
>           aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
>                              false, NULL, NULL, NULL, NULL);
>           nfs_destroy_context(client->context);
> +        client->context = NULL;
>       }
> -    memset(client, 0, sizeof(NFSClient));
> -}
> -
> -static void nfs_file_close(BlockDriverState *bs)
> -{
> -    NFSClient *client = bs->opaque;
> -    nfs_client_close(client);
> +    g_free(client->path);
>       qemu_mutex_destroy(&client->mutex);
> +    qapi_free_NFSServer(client->server);
> +    client->server = NULL;
> +}
> +
> +static void nfs_file_close(BlockDriverState *bs)
> +{
> +    NFSClient *client = bs->opaque;
> +    nfs_client_close(client);
>   }
>   
>   static NFSServer *nfs_config(QDict *options, Error **errp)
> @@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
>       struct stat st;
>       char *file = NULL, *strp = NULL;
>   
> +    qemu_mutex_init(&client->mutex);
>       opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>       qemu_opts_absorb_qdict(opts, options, &local_err);
>       if (local_err) {
> @@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
>       if (ret < 0) {
>           return ret;
>       }
> -    qemu_mutex_init(&client->mutex);
> +
>       bs->total_sectors = ret;
>       ret = 0;
>       return ret;

Reviewed-by: Peter Lieven <pl@kamp.de>

Also CC'ing qemu-stable as this affects 2.9.0 as well.

Peter

Re: [Qemu-devel] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[Kevin Wolf]

Am 08.08.2017 um 00:29 hat Jeff Cody geschrieben:
> Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
> checks for when qemu_mutex() functions are called without the
> corresponding qemu_mutex_init() having initialized the mutex.
> 
> This uncovered a latent bug in qemu's nfs driver - in
> nfs_client_close(), the NFSClient structure is overwritten with zeros,
> prior to the mutex being destroyed.
> 
> Go ahead and destroy the mutex in nfs_client_close(), and change where
> we call qemu_mutex_init() so that it is correctly balanced.
> 
> There are also a couple of memory leaks obscured by the memset, so this
> fixes those as well.
> 
> Finally, we should be able to get rid of the memset(), as it isn't
> necessary.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Thanks, applied to the block branch.

Kevin