[PATCH v3 for-5.1 0/2] Fix crash due to NBD export leak

[PATCH v3 for-5.1 0/2] Fix crash due to NBD export leak

[Vladimir Sementsov-Ogievskiy]

Hi all!

We've faced crash bug, which is reproducing on master branch as well.
The case is described in 01, where fix is suggested.
New iotest in 02 crashes without that fix.

v3: resend for convenience, as all preparatory patches are merged.
01-02: add Eric's r-b and t-b marks

====

This is a crash-fix, so it would be good to fix in 5.1. Still neither
Eric nor I are sure in patch 01: is AIO_WAIT_WHILE used correctly?

====

Side note: this AIO_WAIT_WHILE may be long, if nbd reconnect is enabled
and connection failed recently. Still it's another story: I think we
actually should disable reconnect in bdrv_close, before drain.

Vladimir Sementsov-Ogievskiy (2):
  nbd: make nbd_export_close_all() synchronous
  iotests: test shutdown when bitmap is exported through NBD

 nbd/server.c               |  8 +++++
 tests/qemu-iotests/299     | 65 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/299.out | 10 ++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 84 insertions(+)
 create mode 100644 tests/qemu-iotests/299
 create mode 100644 tests/qemu-iotests/299.out

-- 
2.21.0

[PATCH v3 1/2] nbd: make nbd_export_close_all() synchronous

[Vladimir Sementsov-Ogievskiy]

Consider nbd_export_close_all(). The call-stack looks like this:
 nbd_export_close_all() -> nbd_export_close -> call client_close() for
each client.

client_close() doesn't guarantee that client is closed: nbd_trip()
keeps reference to it. So, nbd_export_close_all() just reduce
reference counter on export and removes it from the list, but doesn't
guarantee that nbd_trip() finished neither export actually removed.

Let's wait for all exports actually removed.

Without this fix, the following crash is possible:

- export bitmap through internal Qemu NBD server
- connect a client
- shutdown Qemu

On shutdown nbd_export_close_all is called, but it actually don't wait
for nbd_trip() to finish and to release its references. So, export is
not release, and exported bitmap remains busy, and on try to remove the
bitmap (which is part of bdrv_close()) the assertion fails:

bdrv_release_dirty_bitmap_locked: Assertion `!bdrv_dirty_bitmap_busy(bitmap)' failed

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index 5357f588f0..4752a6c8bc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -102,6 +102,8 @@ struct NBDExport {
 };
 
 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
+static QTAILQ_HEAD(, NBDExport) closed_exports =
+        QTAILQ_HEAD_INITIALIZER(closed_exports);
 
 /* NBDExportMetaContexts represents a list of contexts to be exported,
  * as selected by NBD_OPT_SET_META_CONTEXT. Also used for
@@ -1659,6 +1661,7 @@ void nbd_export_close(NBDExport *exp)
         g_free(exp->name);
         exp->name = NULL;
         QTAILQ_REMOVE(&exports, exp, next);
+        QTAILQ_INSERT_TAIL(&closed_exports, exp, next);
     }
     g_free(exp->description);
     exp->description = NULL;
@@ -1722,7 +1725,9 @@ void nbd_export_put(NBDExport *exp)
             g_free(exp->export_bitmap_context);
         }
 
+        QTAILQ_REMOVE(&closed_exports, exp, next);
         g_free(exp);
+        aio_wait_kick();
     }
 }
 
@@ -1742,6 +1747,9 @@ void nbd_export_close_all(void)
         nbd_export_close(exp);
         aio_context_release(aio_context);
     }
+
+    AIO_WAIT_WHILE(NULL, !(QTAILQ_EMPTY(&exports) &&
+                           QTAILQ_EMPTY(&closed_exports)));
 }
 
 static int coroutine_fn nbd_co_send_iov(NBDClient *client, struct iovec *iov,
-- 
2.21.0

[PATCH v3 2/2] iotests: test shutdown when bitmap is exported through NBD

[Vladimir Sementsov-Ogievskiy]

Test shutdown when bitmap is exported through NBD and active client
exists. The previous patch fixes a crash, provoked by this scenario.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/299     | 65 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/299.out | 10 ++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 76 insertions(+)
 create mode 100644 tests/qemu-iotests/299
 create mode 100644 tests/qemu-iotests/299.out

diff --git a/tests/qemu-iotests/299 b/tests/qemu-iotests/299
new file mode 100644
index 0000000000..e129c7f7cb
--- /dev/null
+++ b/tests/qemu-iotests/299
@@ -0,0 +1,65 @@
+#!/usr/bin/env python3
+#
+# Test shutdown when bitmap is exported through NBD server
+#
+# Copyright (c) 2020 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+
+# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
+iotests.script_initialize(
+    supported_fmts=['qcow2'],
+)
+
+nbd_sock = iotests.file_path('nbd.sock', base_dir=iotests.sock_dir)
+nbd_uri = 'nbd+unix:///disk?socket=' + nbd_sock
+size = 1024 * 1024
+
+vm = iotests.VM()
+vm.launch()
+
+vm.qmp_log('blockdev-add', **{
+    'node-name': 'disk',
+    'driver': 'null-co',
+    'size': 1024 * 1024,
+})
+
+vm.qmp_log('block-dirty-bitmap-add', **{
+    'node': 'disk',
+    'name': 'bitmap0'
+})
+
+vm.qmp_log('nbd-server-start', **{
+    'addr': {
+        'type': 'unix',
+        'data': {'path': nbd_sock}
+    }
+}, filters=[iotests.filter_qmp_testfiles])
+
+vm.qmp_log('nbd-server-add', **{
+    'device': 'disk',
+    'writable': True,
+    'bitmap': 'bitmap0'
+})
+
+p = iotests.QemuIoInteractive('-f', 'raw', nbd_uri)
+# wait for connection and check it:
+iotests.log(p.cmd('read 0 512').rstrip(), filters=[iotests.filter_qemu_io])
+
+vm.shutdown()
+
+p.close()
diff --git a/tests/qemu-iotests/299.out b/tests/qemu-iotests/299.out
new file mode 100644
index 0000000000..bba4252923
--- /dev/null
+++ b/tests/qemu-iotests/299.out
@@ -0,0 +1,10 @@
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "disk", "size": 1048576}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "disk"}}
+{"return": {}}
+{"execute": "nbd-server-start", "arguments": {"addr": {"data": {"path": "SOCK_DIR/PID-nbd.sock"}, "type": "unix"}}}
+{"return": {}}
+{"execute": "nbd-server-add", "arguments": {"bitmap": "bitmap0", "device": "disk", "writable": true}}
+{"return": {}}
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9b07a7ed03..b320e6688c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -306,3 +306,4 @@
 295 rw
 296 rw
 297 meta
+299 auto quick
-- 
2.21.0

Re: [PATCH v3 for-5.1 0/2] Fix crash due to NBD export leak

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200714162234.13113-1-vsementsov@virtuozzo.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    iotest-qcow2: 022
  TEST    check-unit: tests/test-char
**
ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
ERROR test-char - Bail out! ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 024
  TEST    iotest-qcow2: 025
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=9dc2c7a6e3eb458c88fdd6be6a03c6eb', '-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-fzvixkio/src/docker-src.2020-07-14-12.32.53.19697:/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=9dc2c7a6e3eb458c88fdd6be6a03c6eb
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-fzvixkio/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    16m48.396s
user    0m8.741s


The full log is available at
http://patchew.org/logs/20200714162234.13113-1-vsementsov@virtuozzo.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 v3 for-5.1 0/2] Fix crash due to NBD export leak

[Vladimir Sementsov-Ogievskiy]

14.07.2020 19:49, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200714162234.13113-1-vsementsov@virtuozzo.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    iotest-qcow2: 022
>    TEST    check-unit: tests/test-char
> **
> ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
> ERROR test-char - Bail out! ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
> make: *** [check-unit] Error 1
> make: *** Waiting for unfinished jobs....
>    TEST    iotest-qcow2: 024
>    TEST    iotest-qcow2: 025
> ---
>      raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=9dc2c7a6e3eb458c88fdd6be6a03c6eb', '-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-fzvixkio/src/docker-src.2020-07-14-12.32.53.19697:/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=9dc2c7a6e3eb458c88fdd6be6a03c6eb
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-fzvixkio/src'
> make: *** [docker-run-test-quick@centos7] Error 2
> 
> real    16m48.396s
> user    0m8.741s
> 
> 
> The full log is available at
> http://patchew.org/logs/20200714162234.13113-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 

something unrelated..

-- 
Best regards,
Vladimir

Re: [PATCH v3 for-5.1 0/2] Fix crash due to NBD export leak

[Philippe Mathieu-Daudé]

Cc'ing Marc-André

On 7/14/20 6:49 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200714162234.13113-1-vsementsov@virtuozzo.com/
...
> 
>   TEST    iotest-qcow2: 022
>   TEST    check-unit: tests/test-char
> **
> ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
> ERROR test-char - Bail out! ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL

Seems related to latest chardev-pull-request.

> make: *** [check-unit] Error 1
> make: *** Waiting for unfinished jobs....
>   TEST    iotest-qcow2: 024
>   TEST    iotest-qcow2: 025
> ---
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=9dc2c7a6e3eb458c88fdd6be6a03c6eb', '-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-fzvixkio/src/docker-src.2020-07-14-12.32.53.19697:/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=9dc2c7a6e3eb458c88fdd6be6a03c6eb
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-fzvixkio/src'
> make: *** [docker-run-test-quick@centos7] Error 2
> 
> real    16m48.396s
> user    0m8.741s
> 
> 
> The full log is available at
> http://patchew.org/logs/20200714162234.13113-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.

Re: [PATCH v3 for-5.1 0/2] Fix crash due to NBD export leak

[Kevin Wolf]

Am 14.07.2020 um 18:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> We've faced crash bug, which is reproducing on master branch as well.
> The case is described in 01, where fix is suggested.
> New iotest in 02 crashes without that fix.
> 
> v3: resend for convenience, as all preparatory patches are merged.
> 01-02: add Eric's r-b and t-b marks
> 
> ====
> 
> This is a crash-fix, so it would be good to fix in 5.1. Still neither
> Eric nor I are sure in patch 01: is AIO_WAIT_WHILE used correctly?

Anything specific you had doubts about?

At first sight it looks good to me. It's always called in the main loop
and we don't hold any AioContext locks, so using NULL as the context is
fine. You also made sure to use aio_wait_kick() so that we won't hang
even though the condition has become false.

I'm applying this to my block branch now. If your doubts were about
something more subtle that I missed, we can unstage/revert the patch.

Kevin

Re: [PATCH v3 for-5.1 0/2] Fix crash due to NBD export leak

[Vladimir Sementsov-Ogievskiy]

17.07.2020 15:01, Kevin Wolf wrote:
> Am 14.07.2020 um 18:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hi all!
>>
>> We've faced crash bug, which is reproducing on master branch as well.
>> The case is described in 01, where fix is suggested.
>> New iotest in 02 crashes without that fix.
>>
>> v3: resend for convenience, as all preparatory patches are merged.
>> 01-02: add Eric's r-b and t-b marks
>>
>> ====
>>
>> This is a crash-fix, so it would be good to fix in 5.1. Still neither
>> Eric nor I are sure in patch 01: is AIO_WAIT_WHILE used correctly?
> 
> Anything specific you had doubts about?
> 
> At first sight it looks good to me. It's always called in the main loop
> and we don't hold any AioContext locks, so using NULL as the context is
> fine. You also made sure to use aio_wait_kick() so that we won't hang
> even though the condition has become false.
> 
> I'm applying this to my block branch now. If your doubts were about
> something more subtle that I missed, we can unstage/revert the patch.
> 
> Kevin
> 

Nothing specific, thanks!


-- 
Best regards,
Vladimir