[PATCH 0/2] A pair of memory access problems

[PATCH 0/2] A pair of memory access problems

[Michal Privoznik]

The first patch fixes a crasher, the second fixes a memleak.

Michal Privoznik (2):
  hw/vfio/pci: Fix double free of migration_blocker
  vfio-helpers: Free QEMUVFIOState in qemu_vfio_close()

 hw/vfio/pci.c       | 2 ++
 util/vfio-helpers.c | 1 +
 2 files changed, 3 insertions(+)

-- 
2.23.0

[PATCH 1/2] hw/vfio/pci: Fix double free of migration_blocker

[Michal Privoznik]

When user tries to hotplug a VFIO device, but the operation fails
somewhere in the middle (in my testing it failed because of
RLIMIT_MEMLOCK forbidding more memory allocation), then a double
free occurs. In vfio_realize() the vdev->migration_blocker is
allocated, then something goes wrong which causes control to jump
onto 'error' label where the error is freed. But the pointer is
left pointing to invalid memory. Later, when
vfio_instance_finalize() is called, the memory is freed again.

In my testing the second hunk was sufficient to fix the bug, but
I figured the first hunk doesn't hurt either.

==169952== Invalid read of size 8
==169952==    at 0xA47DCD: error_free (error.c:266)
==169952==    by 0x4E0A18: vfio_instance_finalize (pci.c:3040)
==169952==    by 0x8DF74C: object_deinit (object.c:606)
==169952==    by 0x8DF7BE: object_finalize (object.c:620)
==169952==    by 0x8E0757: object_unref (object.c:1074)
==169952==    by 0x45079C: memory_region_unref (memory.c:1779)
==169952==    by 0x45376B: do_address_space_destroy (memory.c:2793)
==169952==    by 0xA5C600: call_rcu_thread (rcu.c:283)
==169952==    by 0xA427CB: qemu_thread_start (qemu-thread-posix.c:519)
==169952==    by 0x80A8457: start_thread (in /lib64/libpthread-2.29.so)
==169952==    by 0x81C96EE: clone (in /lib64/libc-2.29.so)
==169952==  Address 0x143137e0 is 0 bytes inside a block of size 48 free'd
==169952==    at 0x4A342BB: free (vg_replace_malloc.c:530)
==169952==    by 0xA47E05: error_free (error.c:270)
==169952==    by 0x4E0945: vfio_realize (pci.c:3025)
==169952==    by 0x76A4FF: pci_qdev_realize (pci.c:2099)
==169952==    by 0x689B9A: device_set_realized (qdev.c:876)
==169952==    by 0x8E2C80: property_set_bool (object.c:2080)
==169952==    by 0x8E0EF6: object_property_set (object.c:1272)
==169952==    by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26)
==169952==    by 0x8E11DB: object_property_set_bool (object.c:1338)
==169952==    by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673)
==169952==    by 0x5E81E5: qmp_device_add (qdev-monitor.c:798)
==169952==    by 0x9E18A8: do_qmp_dispatch (qmp-dispatch.c:132)
==169952==  Block was alloc'd at
==169952==    at 0x4A35476: calloc (vg_replace_malloc.c:752)
==169952==    by 0x51B1158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6)
==169952==    by 0xA47357: error_setv (error.c:61)
==169952==    by 0xA475D9: error_setg_internal (error.c:97)
==169952==    by 0x4DF8C2: vfio_realize (pci.c:2737)
==169952==    by 0x76A4FF: pci_qdev_realize (pci.c:2099)
==169952==    by 0x689B9A: device_set_realized (qdev.c:876)
==169952==    by 0x8E2C80: property_set_bool (object.c:2080)
==169952==    by 0x8E0EF6: object_property_set (object.c:1272)
==169952==    by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26)
==169952==    by 0x8E11DB: object_property_set_bool (object.c:1338)
==169952==    by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/vfio/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e6569a7968..9c165995df 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2740,6 +2740,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         if (err) {
             error_propagate(errp, err);
             error_free(vdev->migration_blocker);
+            vdev->migration_blocker = NULL;
             return;
         }
     }
@@ -3023,6 +3024,7 @@ error:
     if (vdev->migration_blocker) {
         migrate_del_blocker(vdev->migration_blocker);
         error_free(vdev->migration_blocker);
+        vdev->migration_blocker = NULL;
     }
 }
 
-- 
2.23.0

Re: [PATCH 1/2] hw/vfio/pci: Fix double free of migration_blocker

[Cornelia Huck]

On Mon, 11 Nov 2019 11:37:41 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> When user tries to hotplug a VFIO device, but the operation fails
> somewhere in the middle (in my testing it failed because of
> RLIMIT_MEMLOCK forbidding more memory allocation), then a double
> free occurs. In vfio_realize() the vdev->migration_blocker is
> allocated, then something goes wrong which causes control to jump
> onto 'error' label where the error is freed. But the pointer is
> left pointing to invalid memory. Later, when
> vfio_instance_finalize() is called, the memory is freed again.
> 
> In my testing the second hunk was sufficient to fix the bug, but
> I figured the first hunk doesn't hurt either.

Better safe than sorry, I guess.

> 
> ==169952== Invalid read of size 8
> ==169952==    at 0xA47DCD: error_free (error.c:266)
> ==169952==    by 0x4E0A18: vfio_instance_finalize (pci.c:3040)
> ==169952==    by 0x8DF74C: object_deinit (object.c:606)
> ==169952==    by 0x8DF7BE: object_finalize (object.c:620)
> ==169952==    by 0x8E0757: object_unref (object.c:1074)
> ==169952==    by 0x45079C: memory_region_unref (memory.c:1779)
> ==169952==    by 0x45376B: do_address_space_destroy (memory.c:2793)
> ==169952==    by 0xA5C600: call_rcu_thread (rcu.c:283)
> ==169952==    by 0xA427CB: qemu_thread_start (qemu-thread-posix.c:519)
> ==169952==    by 0x80A8457: start_thread (in /lib64/libpthread-2.29.so)
> ==169952==    by 0x81C96EE: clone (in /lib64/libc-2.29.so)
> ==169952==  Address 0x143137e0 is 0 bytes inside a block of size 48 free'd
> ==169952==    at 0x4A342BB: free (vg_replace_malloc.c:530)
> ==169952==    by 0xA47E05: error_free (error.c:270)
> ==169952==    by 0x4E0945: vfio_realize (pci.c:3025)
> ==169952==    by 0x76A4FF: pci_qdev_realize (pci.c:2099)
> ==169952==    by 0x689B9A: device_set_realized (qdev.c:876)
> ==169952==    by 0x8E2C80: property_set_bool (object.c:2080)
> ==169952==    by 0x8E0EF6: object_property_set (object.c:1272)
> ==169952==    by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26)
> ==169952==    by 0x8E11DB: object_property_set_bool (object.c:1338)
> ==169952==    by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673)
> ==169952==    by 0x5E81E5: qmp_device_add (qdev-monitor.c:798)
> ==169952==    by 0x9E18A8: do_qmp_dispatch (qmp-dispatch.c:132)
> ==169952==  Block was alloc'd at
> ==169952==    at 0x4A35476: calloc (vg_replace_malloc.c:752)
> ==169952==    by 0x51B1158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6)
> ==169952==    by 0xA47357: error_setv (error.c:61)
> ==169952==    by 0xA475D9: error_setg_internal (error.c:97)
> ==169952==    by 0x4DF8C2: vfio_realize (pci.c:2737)
> ==169952==    by 0x76A4FF: pci_qdev_realize (pci.c:2099)
> ==169952==    by 0x689B9A: device_set_realized (qdev.c:876)
> ==169952==    by 0x8E2C80: property_set_bool (object.c:2080)
> ==169952==    by 0x8E0EF6: object_property_set (object.c:1272)
> ==169952==    by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26)
> ==169952==    by 0x8E11DB: object_property_set_bool (object.c:1338)
> ==169952==    by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Fixes: f045a0104c8c ("vfio: unplug failover primary device before migration")

> ---
>  hw/vfio/pci.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH 2/2] vfio-helpers: Free QEMUVFIOState in qemu_vfio_close()

[Michal Privoznik]

The qemu_vfio_open_pci() allocates this QEMUVFIOState structure
but free counterpart is missing. Since we already have
qemu_vfio_close() which does cleanup of the state, it looks like
a perfect place to free the structure too.

==178278== 528 (360 direct, 168 indirect) bytes in 1 blocks are definitely lost in loss record 6,605 of 6,985
==178278==    at 0x4A35476: calloc (vg_replace_malloc.c:752)
==178278==    by 0x51B1158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6)
==178278==    by 0xA68613: qemu_vfio_open_pci (vfio-helpers.c:428)
==178278==    by 0x9779EA: nvme_init (nvme.c:606)
==178278==    by 0x97830F: nvme_file_open (nvme.c:795)
==178278==    by 0x8E9439: bdrv_open_driver (block.c:1293)
==178278==    by 0x8E9E1C: bdrv_open_common (block.c:1553)
==178278==    by 0x8ED264: bdrv_open_inherit (block.c:3083)
==178278==    by 0x8ED79D: bdrv_open (block.c:3176)
==178278==    by 0x5DA5C1: bds_tree_init (blockdev.c:670)
==178278==    by 0x5E2B64: qmp_blockdev_add (blockdev.c:4354)
==178278==    by 0x5ECB1D: configure_blockdev (vl.c:1202)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 util/vfio-helpers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 813f7ec564..5ff91c1e5c 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -721,4 +721,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
     close(s->device);
     close(s->group);
     close(s->container);
+    g_free(s);
 }
-- 
2.23.0

Re: [PATCH 2/2] vfio-helpers: Free QEMUVFIOState in qemu_vfio_close()

[Cornelia Huck]

On Mon, 11 Nov 2019 11:37:42 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> The qemu_vfio_open_pci() allocates this QEMUVFIOState structure
> but free counterpart is missing. Since we already have
> qemu_vfio_close() which does cleanup of the state, it looks like
> a perfect place to free the structure too.
> 
> ==178278== 528 (360 direct, 168 indirect) bytes in 1 blocks are definitely lost in loss record 6,605 of 6,985
> ==178278==    at 0x4A35476: calloc (vg_replace_malloc.c:752)
> ==178278==    by 0x51B1158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6)
> ==178278==    by 0xA68613: qemu_vfio_open_pci (vfio-helpers.c:428)
> ==178278==    by 0x9779EA: nvme_init (nvme.c:606)
> ==178278==    by 0x97830F: nvme_file_open (nvme.c:795)
> ==178278==    by 0x8E9439: bdrv_open_driver (block.c:1293)
> ==178278==    by 0x8E9E1C: bdrv_open_common (block.c:1553)
> ==178278==    by 0x8ED264: bdrv_open_inherit (block.c:3083)
> ==178278==    by 0x8ED79D: bdrv_open (block.c:3176)
> ==178278==    by 0x5DA5C1: bds_tree_init (blockdev.c:670)
> ==178278==    by 0x5E2B64: qmp_blockdev_add (blockdev.c:4354)
> ==178278==    by 0x5ECB1D: configure_blockdev (vl.c:1202)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  util/vfio-helpers.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 813f7ec564..5ff91c1e5c 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -721,4 +721,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
>      close(s->device);
>      close(s->group);
>      close(s->container);
> +    g_free(s);

Not sure if freeing the parameter passed in via a function called
'close' isn't too surprising... it's not that obvious that the caller
is also relinquishing its reference to the QEMUVFIOState; maybe rename
the function to qemu_vfio_close_and_free() or so?

[Looking at the blockdev code, it uses the pattern of first closing the
bs and then freeing it separately, which is a bit odd as the only call
to bdrv_close (which will eventually end up here for nvme) is
immediately followed by a g_free. Just something I noticed.]

>  }

Re: [PATCH 2/2] vfio-helpers: Free QEMUVFIOState in qemu_vfio_close()

[Michal Privoznik]

On 11/11/19 12:15 PM, Cornelia Huck wrote:
> On Mon, 11 Nov 2019 11:37:42 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> The qemu_vfio_open_pci() allocates this QEMUVFIOState structure
>> but free counterpart is missing. Since we already have
>> qemu_vfio_close() which does cleanup of the state, it looks like
>> a perfect place to free the structure too.
>>
>> ==178278== 528 (360 direct, 168 indirect) bytes in 1 blocks are definitely lost in loss record 6,605 of 6,985
>> ==178278==    at 0x4A35476: calloc (vg_replace_malloc.c:752)
>> ==178278==    by 0x51B1158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6)
>> ==178278==    by 0xA68613: qemu_vfio_open_pci (vfio-helpers.c:428)
>> ==178278==    by 0x9779EA: nvme_init (nvme.c:606)
>> ==178278==    by 0x97830F: nvme_file_open (nvme.c:795)
>> ==178278==    by 0x8E9439: bdrv_open_driver (block.c:1293)
>> ==178278==    by 0x8E9E1C: bdrv_open_common (block.c:1553)
>> ==178278==    by 0x8ED264: bdrv_open_inherit (block.c:3083)
>> ==178278==    by 0x8ED79D: bdrv_open (block.c:3176)
>> ==178278==    by 0x5DA5C1: bds_tree_init (blockdev.c:670)
>> ==178278==    by 0x5E2B64: qmp_blockdev_add (blockdev.c:4354)
>> ==178278==    by 0x5ECB1D: configure_blockdev (vl.c:1202)
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   util/vfio-helpers.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index 813f7ec564..5ff91c1e5c 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -721,4 +721,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
>>       close(s->device);
>>       close(s->group);
>>       close(s->container);
>> +    g_free(s);
> 
> Not sure if freeing the parameter passed in via a function called
> 'close' isn't too surprising... it's not that obvious that the caller
> is also relinquishing its reference to the QEMUVFIOState; maybe rename
> the function to qemu_vfio_close_and_free() or so?

Alright, I'll rename the function. I worry that if free is left as an 
exercise to caller then it'll be always forgotten about. That's why I 
put the call into close function.

Michal

Re: [PATCH 0/2] A pair of memory access problems

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1573468531.git.mprivozn@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-unit: tests/test-thread-pool
  TEST    check-unit: tests/test-hbitmap
**
ERROR:/tmp/qemu-test/src/tests/migration-test.c:903:wait_for_migration_fail: assertion failed: (!strcmp(status, "setup") || !strcmp(status, "failed") || (allow_active && !strcmp(status, "active")))
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/migration-test.c:903:wait_for_migration_fail: assertion failed: (!strcmp(status, "setup") || !strcmp(status, "failed") || (allow_active && !strcmp(status, "active")))
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-unit: tests/test-bdrv-drain
  TEST    check-unit: tests/test-bdrv-graph-mod
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=e993b5a80ea14863bad17c8751205f9e', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-_b06sxmc/src/docker-src.2019-11-11-09.03.28.17585:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=e993b5a80ea14863bad17c8751205f9e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_b06sxmc/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    10m21.635s
user    0m8.000s


The full log is available at
http://patchew.org/logs/cover.1573468531.git.mprivozn@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] A pair of memory access problems

[Alex Williamson]

On Mon, 11 Nov 2019 11:37:40 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> The first patch fixes a crasher, the second fixes a memleak.
> 
> Michal Privoznik (2):
>   hw/vfio/pci: Fix double free of migration_blocker
>   vfio-helpers: Free QEMUVFIOState in qemu_vfio_close()
> 
>  hw/vfio/pci.c       | 2 ++
>  util/vfio-helpers.c | 1 +
>  2 files changed, 3 insertions(+)

I'll take the first patch, looks like there's some discussion still on
the vfio-helpers patch and I don't own that file anyway, so please post
it separately.  Thanks,

Alex