[PATCH v2 0/5] Fix crash due to NBD export leak

[PATCH v2 0/5] 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 03, where fix is suggested.
New iotest in 05 crashes without that fix.

v2:
01: reword commit msg, add Eric's r-b
02: fix type in commit msg, add Eric's r-b
03: rewrite
v1:04 split into 04 (iotests.py improvement) and 05(new test)


Vladimir Sementsov-Ogievskiy (5):
  iotests: QemuIoInteractive: use qemu_io_args_no_fmt
  iotests.py: QemuIoInteractive: print output on failure
  nbd: make nbd_export_close_all() synchronous
  iotests.py: filter_testfiles(): filter SOCK_DIR too
  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 +
 tests/qemu-iotests/iotests.py | 15 +++++---
 5 files changed, 95 insertions(+), 4 deletions(-)
 create mode 100644 tests/qemu-iotests/299
 create mode 100644 tests/qemu-iotests/299.out

-- 
2.18.0

[PATCH v2 1/5] iotests: QemuIoInteractive: use qemu_io_args_no_fmt

[Vladimir Sementsov-Ogievskiy]

The only user (iotest 205) of QemuIoInteractive provides -f argument,
so it's a bit inefficient to use qemu_io_args, which contains -f too.
And we are going to add one more test, which wants to specify -f by
hand. Let's use qemu_io_args_no_fmt.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5ea4c4df8b..efe9958f5e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -211,7 +211,7 @@ def get_virtio_scsi_device():
 
 class QemuIoInteractive:
     def __init__(self, *args):
-        self.args = qemu_io_args + list(args)
+        self.args = qemu_io_args_no_fmt + list(args)
         self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
                                    stdout=subprocess.PIPE,
                                    stderr=subprocess.STDOUT,
-- 
2.18.0

[PATCH v2 2/5] iotests.py: QemuIoInteractive: print output on failure

[Vladimir Sementsov-Ogievskiy]

Make it simpler to debug when qemu-io fails due to wrong arguments or
environment.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/iotests.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index efe9958f5e..ac9d199a1e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -216,7 +216,13 @@ class QemuIoInteractive:
                                    stdout=subprocess.PIPE,
                                    stderr=subprocess.STDOUT,
                                    universal_newlines=True)
-        assert self._p.stdout.read(9) == 'qemu-io> '
+        out = self._p.stdout.read(9)
+        if out != 'qemu-io> ':
+            # Most probably qemu-io just failed to start.
+            # Let's collect the whole output and exit.
+            out += self._p.stdout.read()
+            self._p.wait(timeout=1)
+            raise ValueError(out)
 
     def close(self):
         self._p.communicate('q\n')
-- 
2.18.0

[PATCH v2 3/5] 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>
---

v2: rewritten, try to wait exports directly.

Note: I'm not sure in my understanding of AIO_WAIT_WHILE and related things
and really hope for review.


 nbd/server.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index 20754e9ebc..9d64b00f4b 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
@@ -1655,6 +1657,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;
@@ -1717,7 +1720,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();
     }
 }
 
@@ -1737,6 +1742,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.18.0

[PATCH v2 4/5] iotests.py: filter_testfiles(): filter SOCK_DIR too

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ac9d199a1e..31d4b105ca 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -345,8 +345,9 @@ def filter_qmp(qmsg, filter_fn):
     return qmsg
 
 def filter_testfiles(msg):
-    prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
-    return msg.replace(prefix, 'TEST_DIR/PID-')
+    pref1 = os.path.join(test_dir, "%s-" % (os.getpid()))
+    pref2 = os.path.join(sock_dir, "%s-" % (os.getpid()))
+    return msg.replace(pref1, 'TEST_DIR/PID-').replace(pref2, 'SOCK_DIR/PID-')
 
 def filter_qmp_testfiles(qmsg):
     def _filter(_key, value):
-- 
2.18.0

[PATCH v2 5/5] 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>
---
 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 d886fa0cb3..250192352c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -302,3 +302,4 @@
 291 rw quick
 292 rw auto quick
 297 meta
+299 auto quick
-- 
2.18.0

Re: [PATCH v2 3/5] nbd: make nbd_export_close_all() synchronous

[Eric Blake]

On 7/1/20 5:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
> 
> v2: rewritten, try to wait exports directly.
> 
> Note: I'm not sure in my understanding of AIO_WAIT_WHILE and related things
> and really hope for review.

I'm also a bit weak on whether the AIO_WAIT_WHILE is being used 
correctly.  But the idea behind the patch makes sense to me, and since 
it is a bug fix, it will be okay to apply this for -rc1 or even -rc2 if 
needed (I'm not including it in my pull request today, however).

> 
> 
>   nbd/server.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 20754e9ebc..9d64b00f4b 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
> @@ -1655,6 +1657,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;
> @@ -1717,7 +1720,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();
>       }
>   }
>   
> @@ -1737,6 +1742,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,
> 

weak:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 4/5] iotests.py: filter_testfiles(): filter SOCK_DIR too

[Eric Blake]

On 7/1/20 5:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/iotests.py | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index ac9d199a1e..31d4b105ca 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -345,8 +345,9 @@ def filter_qmp(qmsg, filter_fn):
>       return qmsg
>   
>   def filter_testfiles(msg):
> -    prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
> -    return msg.replace(prefix, 'TEST_DIR/PID-')
> +    pref1 = os.path.join(test_dir, "%s-" % (os.getpid()))
> +    pref2 = os.path.join(sock_dir, "%s-" % (os.getpid()))
> +    return msg.replace(pref1, 'TEST_DIR/PID-').replace(pref2, 'SOCK_DIR/PID-')
>   
>   def filter_qmp_testfiles(qmsg):
>       def _filter(_key, value):
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 5/5] iotests: test shutdown when bitmap is exported through NBD

[Eric Blake]

On 7/1/20 5:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
>   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

I confirmed that by temporarily applying this out of order (well, 
sinking 3/5 last) does indeed provoke the failure, so:

Tested-by: Eric Blake <eblake@redhat.com>

and the test is simple enough:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 4/5] iotests.py: filter_testfiles(): filter SOCK_DIR too

[Eric Blake]

On 7/13/20 8:07 AM, Eric Blake wrote:
> On 7/1/20 5:53 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Queuing 1, 2, and 4 through my NBD tree as trivial iotest improvements, 
while we await better review on 3.

> 
>> diff --git a/tests/qemu-iotests/iotests.py 
>> b/tests/qemu-iotests/iotests.py
>> index ac9d199a1e..31d4b105ca 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -345,8 +345,9 @@ def filter_qmp(qmsg, filter_fn):
>>       return qmsg
>>   def filter_testfiles(msg):
>> -    prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>> -    return msg.replace(prefix, 'TEST_DIR/PID-')
>> +    pref1 = os.path.join(test_dir, "%s-" % (os.getpid()))
>> +    pref2 = os.path.join(sock_dir, "%s-" % (os.getpid()))
>> +    return msg.replace(pref1, 'TEST_DIR/PID-').replace(pref2, 
>> 'SOCK_DIR/PID-')
>>   def filter_qmp_testfiles(qmsg):
>>       def _filter(_key, value):
>>
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 4/5] iotests.py: filter_testfiles(): filter SOCK_DIR too

[Vladimir Sementsov-Ogievskiy]

13.07.2020 17:00, Eric Blake wrote:
> On 7/13/20 8:07 AM, Eric Blake wrote:
>> On 7/1/20 5:53 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/iotests.py | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Queuing 1, 2, and 4 through my NBD tree as trivial iotest improvements, while we await better review on 3.

we aio_wait :)

Thanks!

> 
>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index ac9d199a1e..31d4b105ca 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -345,8 +345,9 @@ def filter_qmp(qmsg, filter_fn):
>>>       return qmsg
>>>   def filter_testfiles(msg):
>>> -    prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>>> -    return msg.replace(prefix, 'TEST_DIR/PID-')
>>> +    pref1 = os.path.join(test_dir, "%s-" % (os.getpid()))
>>> +    pref2 = os.path.join(sock_dir, "%s-" % (os.getpid()))
>>> +    return msg.replace(pref1, 'TEST_DIR/PID-').replace(pref2, 'SOCK_DIR/PID-')
>>>   def filter_qmp_testfiles(qmsg):
>>>       def _filter(_key, value):
>>>
>>
> 


-- 
Best regards,
Vladimir