* [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver @ 2021-02-11 14:26 Philippe Mathieu-Daudé 2021-02-11 14:26 ` [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' Philippe Mathieu-Daudé ` (3 more replies) 0 siblings, 4 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-11 14:26 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf The null-co driver doesn't zeroize buffer in its default config, because it is designed for testing and tests want to run fast. However this confuses security researchers (access to uninit buffers). A one-line patch supposed which became a painful one, because there is so many different syntax to express the same usage: opt = qdict_new(); qdict_put_str(opt, "read-zeroes", "off"); null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL, &error_abort); vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...) vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none") blk0 = { 'node-name': 'src', 'driver': 'null-co', 'read-zeroes': 'off' } 'file': { 'driver': 'null-co', 'read-zeroes': False, } "file": { "driver": "null-co", "read-zeroes": "off" } { "execute": "blockdev-add", "arguments": { "driver": "null-co", "read-zeroes": false, "node-name": "disk0" } } opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024} qemu -drive driver=null-co,read-zeroes=off qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}" qemu-img null-co://,read-zeroes=off qemu-img ... -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}" There are probably more. Anyhow, the iotests I am not sure and should be audited are 056, 155 (I don't understand the syntax) and 162. Please review, Phil. Philippe Mathieu-Daud=C3=A9 (2): block: Explicit null-co uses 'read-zeroes=3Dfalse' block/null: Enable 'read-zeroes' mode by default docs/devel/testing.rst | 14 +++++++------- tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- block/null.c | 2 +- tests/test-bdrv-drain.c | 10 ++++++++-- tests/acceptance/virtio_check_params.py | 2 +- tests/perf/block/qcow2/convert-blockstatus | 6 +++--- tests/qemu-iotests/040 | 2 +- tests/qemu-iotests/041 | 12 ++++++++---- tests/qemu-iotests/051 | 2 +- tests/qemu-iotests/051.out | 2 +- tests/qemu-iotests/051.pc.out | 4 ++-- tests/qemu-iotests/087 | 6 ++++-- tests/qemu-iotests/118 | 2 +- tests/qemu-iotests/133 | 2 +- tests/qemu-iotests/153 | 8 ++++---- tests/qemu-iotests/184 | 2 ++ tests/qemu-iotests/184.out | 10 +++++----- tests/qemu-iotests/218 | 3 +++ tests/qemu-iotests/224 | 3 ++- tests/qemu-iotests/224.out | 8 ++++---- tests/qemu-iotests/225 | 2 +- tests/qemu-iotests/227 | 4 ++-- tests/qemu-iotests/227.out | 4 ++-- tests/qemu-iotests/228 | 2 +- tests/qemu-iotests/235 | 1 + tests/qemu-iotests/245 | 2 +- tests/qemu-iotests/270 | 2 +- tests/qemu-iotests/283 | 3 ++- tests/qemu-iotests/283.out | 4 ++-- tests/qemu-iotests/299 | 1 + tests/qemu-iotests/299.out | 2 +- tests/qemu-iotests/300 | 4 ++-- 32 files changed, 82 insertions(+), 60 deletions(-) --=20 2.26.2 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' 2021-02-11 14:26 [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Philippe Mathieu-Daudé @ 2021-02-11 14:26 ` Philippe Mathieu-Daudé 2021-02-11 16:29 ` Vladimir Sementsov-Ogievskiy ` (2 more replies) 2021-02-11 14:26 ` [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default Philippe Mathieu-Daudé ` (2 subsequent siblings) 3 siblings, 3 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-11 14:26 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf We are going to switch the 'null-co' default 'read-zeroes' value from FALSE to TRUE in the next commit. First explicit the FALSE value when it is not set. Suggested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- - Missing: 056 & 155. I couldn't figure out the proper syntax, any help welcomed... - I'm unsure about 162, this doesn't seem to use the null-co driver but rather testing global syntax. --- docs/devel/testing.rst | 14 +++++++------- tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- tests/test-bdrv-drain.c | 10 ++++++++-- tests/acceptance/virtio_check_params.py | 2 +- tests/perf/block/qcow2/convert-blockstatus | 6 +++--- tests/qemu-iotests/040 | 2 +- tests/qemu-iotests/041 | 12 ++++++++---- tests/qemu-iotests/051 | 2 +- tests/qemu-iotests/051.out | 2 +- tests/qemu-iotests/051.pc.out | 4 ++-- tests/qemu-iotests/087 | 6 ++++-- tests/qemu-iotests/118 | 2 +- tests/qemu-iotests/133 | 2 +- tests/qemu-iotests/153 | 8 ++++---- tests/qemu-iotests/184 | 2 ++ tests/qemu-iotests/184.out | 10 +++++----- tests/qemu-iotests/218 | 3 +++ tests/qemu-iotests/224 | 3 ++- tests/qemu-iotests/224.out | 8 ++++---- tests/qemu-iotests/225 | 2 +- tests/qemu-iotests/227 | 4 ++-- tests/qemu-iotests/227.out | 4 ++-- tests/qemu-iotests/228 | 2 +- tests/qemu-iotests/235 | 1 + tests/qemu-iotests/245 | 2 +- tests/qemu-iotests/270 | 2 +- tests/qemu-iotests/283 | 3 ++- tests/qemu-iotests/283.out | 4 ++-- tests/qemu-iotests/299 | 1 + tests/qemu-iotests/299.out | 2 +- tests/qemu-iotests/300 | 4 ++-- 31 files changed, 81 insertions(+), 59 deletions(-) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 209f9d8172f..45f1a674384 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -216,13 +216,13 @@ code. Both Python and Bash frameworks in iotests provide helpers to manage test images. They can be used to create and clean up images under the test directory. If no I/O or any protocol specific feature is needed, it is often -more convenient to use the pseudo block driver, ``null-co://``, as the test -image, which doesn't require image creation or cleaning up. Avoid system-wide -devices or files whenever possible, such as ``/dev/null`` or ``/dev/zero``. -Otherwise, image locking implications have to be considered. For example, -another application on the host may have locked the file, possibly leading to a -test failure. If using such devices are explicitly desired, consider adding -``locking=off`` option to disable image locking. +more convenient to use the pseudo block driver, ``null-co://,read-zeroes=off``, +as the test image, which doesn't require image creation or cleaning up. Avoid +system-wide devices or files whenever possible, such as ``/dev/null`` or +``/dev/zero``. Otherwise, image locking implications have to be considered. +For example, another application on the host may have locked the file, possibly +leading to a test failure. If using such devices are explicitly desired, +consider adding ``locking=off`` option to disable image locking. Test case groups ---------------- diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h index 5d599765c4b..dd5a7aeff0d 100644 --- a/tests/qtest/fuzz/generic_fuzz_configs.h +++ b/tests/qtest/fuzz/generic_fuzz_configs.h @@ -38,13 +38,13 @@ const generic_fuzz_config predefined_configs[] = { },{ .name = "virtio-blk", .args = "-machine q35 -device virtio-blk,drive=disk0 " - "-drive file=null-co://,id=disk0,if=none,format=raw", + "-drive file=null-co://,file.read-zeroes=off,id=disk0,if=none,format=raw", .objects = "virtio*", },{ .name = "virtio-scsi", .args = "-machine q35 -device virtio-scsi,num_queues=8 " "-device scsi-hd,drive=disk0 " - "-drive file=null-co://,id=disk0,if=none,format=raw", + "-drive file=null-co://,file.read-zeroes=off,id=disk0,if=none,format=raw", .objects = "scsi* virtio*", },{ .name = "virtio-gpu", @@ -119,7 +119,7 @@ const generic_fuzz_config predefined_configs[] = { },{ .name = "ahci-hd", .args = "-machine q35 -nodefaults " - "-drive file=null-co://,if=none,format=raw,id=disk0 " + "-drive file=null-co://,file.read-zeroes=off,if=none,format=raw,id=disk0 " "-device ide-hd,drive=disk0", .objects = "*ahci*", },{ @@ -137,7 +137,7 @@ const generic_fuzz_config predefined_configs[] = { },{ .name = "xhci", .args = "-machine q35 -nodefaults " - "-drive file=null-co://,if=none,format=raw,id=disk0 " + "-drive file=null-co://,file.read-zeroes=off,if=none,format=raw,id=disk0 " "-device qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0 " "-device usb-bot -device usb-storage,drive=disk0 " "-chardev null,id=cd0 -chardev null,id=cd1 " @@ -182,7 +182,8 @@ const generic_fuzz_config predefined_configs[] = { .name = "sdhci-v3", .args = "-nodefaults -device sdhci-pci,sd-spec-version=3 " "-device sd-card,drive=mydrive " - "-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive -nographic", + "-drive if=sd,index=0,file=null-co://,file.read-zeroes=off," + "format=raw,id=mydrive -nographic", .objects = "sd*" },{ .name = "ehci", diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index 8a29e33e004..a3e7d872d88 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -27,6 +27,7 @@ #include "block/blockjob_int.h" #include "sysemu/block-backend.h" #include "qapi/error.h" +#include "qapi/qmp/qdict.h" #include "qemu/main-loop.h" #include "iothread.h" @@ -1177,13 +1178,16 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete, BDRVTestTopState *tts; TestCoDeleteByDrainData dbdd; Coroutine *co; + QDict *opt; bs = bdrv_new_open_driver(&bdrv_test_top_driver, "top", BDRV_O_RDWR, &error_abort); bs->total_sectors = 65536 >> BDRV_SECTOR_BITS; tts = bs->opaque; - null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, + opt = qdict_new(); + qdict_put_str(opt, "read-zeroes", "off"); + null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL, &error_abort); bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds, BDRV_CHILD_DATA, &error_abort); @@ -1201,7 +1205,9 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete, /* This child is just there to be deleted * (for detach_instead_of_delete == true) */ - null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, + opt = qdict_new(); + qdict_put_str(opt, "read-zeroes", "off"); + null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL, &error_abort); bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds, BDRV_CHILD_DATA, &error_abort); diff --git a/tests/acceptance/virtio_check_params.py b/tests/acceptance/virtio_check_params.py index 87e6c839d14..dee386d26f4 100644 --- a/tests/acceptance/virtio_check_params.py +++ b/tests/acceptance/virtio_check_params.py @@ -38,7 +38,7 @@ 'virtio-blk-pci': ['-device', 'virtio-blk-pci,id=scsi0,drive=drive0', '-drive', - 'driver=null-co,id=drive0,if=none']} + 'driver=null-co,read-zeroes=off,id=drive0,if=none']} class VirtioMaxSegSettingsCheck(Test): diff --git a/tests/perf/block/qcow2/convert-blockstatus b/tests/perf/block/qcow2/convert-blockstatus index a1a3c1ef438..c7449eb2c38 100755 --- a/tests/perf/block/qcow2/convert-blockstatus +++ b/tests/perf/block/qcow2/convert-blockstatus @@ -46,7 +46,7 @@ done | $QEMU_IO "$src" ) > /dev/null echo -n "plain: " -/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co:// +/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://,read-zeroes=off # test-case forward @@ -61,11 +61,11 @@ done | $QEMU_IO "$src" ) > /dev/null echo -n "forward: " -/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co:// +/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://,read-zeroes=off # test-case prealloc $QEMU_IMG create -f qcow2 -o preallocation=metadata "$src" $size > /dev/null echo -n "prealloc: " -/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co:// +/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://,read-zeroes=off diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index 7ebc9ed8257..3abc653e599 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -220,7 +220,7 @@ class TestSingleDrive(ImageCommitTestCase): def test_top_node_in_wrong_chain(self): self.assert_no_active_block_jobs() - result = self.vm.qmp('blockdev-add', driver='null-co', node_name='null') + result = self.vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, node_name='null') self.assert_qmp(result, 'return', {}) result = self.vm.qmp('block-commit', device='drive0', top_node='null', base_node='base') diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index 5cc02b24fc7..74d2bec02a8 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -1159,13 +1159,16 @@ class TestRepairQuorum(iotests.QMPTestCase): class TestOrphanedSource(iotests.QMPTestCase): def setUp(self): blk0 = { 'node-name': 'src', - 'driver': 'null-co' } + 'driver': 'null-co', + 'read-zeroes': 'off' } blk1 = { 'node-name': 'dest', - 'driver': 'null-co' } + 'driver': 'null-co', + 'read-zeroes': 'off' } blk2 = { 'node-name': 'dest-ro', 'driver': 'null-co', + 'read-zeroes': 'off', 'read-only': 'on' } self.vm = iotests.VM() @@ -1272,14 +1275,15 @@ class TestReplaces(iotests.QMPTestCase): 'driver': 'copy-on-read', 'node-name': 'filter1', 'file': { - 'driver': 'null-co' + 'driver': 'null-co', + 'read-zeroes': False, } } }) self.assert_qmp(result, 'return', {}) result = self.vm.qmp('blockdev-add', - node_name='target', driver='null-co') + node_name='target', driver='null-co', read_zeroes=False) self.assert_qmp(result, 'return', {}) result = self.vm.qmp('blockdev-mirror', job_id='mirror', device='filter0', diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index 7cbd1415ce7..42e69054f9f 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -384,7 +384,7 @@ if [ "${VALGRIND_QEMU_VM}" == "y" ]; then _casenotrun "Valgrind needs a valid TMPDIR for itself" fi VALGRIND_QEMU_VM= \ -TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on +TMPDIR=/nonexistent run_qemu -drive driver=null-co,read-zeroes=off,snapshot=on # Using snapshot=on together with read-only=on echo "info block" | diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index de4771bcb36..a4bf0cc4401 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -459,7 +459,7 @@ wrote 4096/4096 bytes at offset 0 read 4096/4096 bytes at offset 0 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Testing: -drive driver=null-co,snapshot=on -QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory +QEMU_PROG: -drive driver=null-co,read-zeroes=off,snapshot=on: Could not get temporary filename: No such file or directory Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on,read-only=on,if=none,id=drive0 QEMU X.Y.Z monitor - type 'help' for more information diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index f707471fb00..12275015eaf 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -558,8 +558,8 @@ wrote 4096/4096 bytes at offset 0 read 4096/4096 bytes at offset 0 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Testing: -drive driver=null-co,snapshot=on -QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory +Testing: -drive driver=null-co,read-zeroes=off,snapshot=on +QEMU_PROG: -drive driver=null-co,read-zeroes=off,snapshot=on: Could not get temporary filename: No such file or directory Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on,read-only=on,if=none,id=drive0 QEMU X.Y.Z monitor - type 'help' for more information diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index edd43f1a281..93549524277 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -89,7 +89,8 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO "driver": "$IMGFMT", "node-name": "disk", "file": { - "driver": "null-co" + "driver": "null-co", + "read-zeroes": "off" } } } @@ -98,7 +99,8 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO "driver": "$IMGFMT", "node-name": "test-node", "file": { - "driver": "null-co" + "driver": "null-co", + "read-zeroes": "off" } } } diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index cae52ffa5ea..2795d7b9edd 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -629,7 +629,7 @@ class TestBlockJobsAfterCycle(ChangeBaseClass): qemu_img('create', '-f', iotests.imgfmt, old_img, '1440K') self.vm = iotests.VM() - self.vm.add_drive_raw("id=drive0,driver=null-co,if=none") + self.vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none") self.vm.add_device('floppy,drive=drive0,id=%s' % self.device_name) self.vm.launch() diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133 index d997db16855..fde80d5ac6c 100755 --- a/tests/qemu-iotests/133 +++ b/tests/qemu-iotests/133 @@ -89,7 +89,7 @@ echo # Using the json: pseudo-protocol we can create non-string options # (Invoke 'info' just so we get some output afterwards) IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \ - "json:{'driver': 'null-co', 'size': 65536}" + "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}" echo echo "=== Check that mixing -c/-r/-w and their corresponding options is forbidden ===" diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153 index 607af590918..10299c0ab44 100755 --- a/tests/qemu-iotests/153 +++ b/tests/qemu-iotests/153 @@ -261,17 +261,17 @@ echo "== Detecting -U and force-share conflicts ==" echo echo 'No conflict:' -$QEMU_IMG info -U --image-opts driver=null-co,force-share=on +$QEMU_IMG info -U --image-opts driver=null-co,read-zeroes=off,force-share=on echo echo 'Conflict:' -$QEMU_IMG info -U --image-opts driver=null-co,force-share=off +$QEMU_IMG info -U --image-opts driver=null-co,read-zeroes=off,force-share=off echo echo 'No conflict:' -$QEMU_IO -c 'open -r -U -o driver=null-co,force-share=on' +$QEMU_IO -c 'open -r -U -o driver=null-co,read-zeroes=off,force-share=on' echo echo 'Conflict:' -$QEMU_IO -c 'open -r -U -o driver=null-co,force-share=off' +$QEMU_IO -c 'open -r -U -o driver=null-co,read-zeroes=off,force-share=off' # success, all done echo "*** done" diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184 index 513d167098e..1a29e2430e2 100755 --- a/tests/qemu-iotests/184 +++ b/tests/qemu-iotests/184 @@ -60,6 +60,7 @@ run_qemu <<EOF { "execute": "blockdev-add", "arguments": { "driver": "null-co", + "read-zeroes": false, "node-name": "disk0" } } @@ -171,6 +172,7 @@ run_qemu <<EOF { "execute": "blockdev-add", "arguments": { "driver": "null-co", + "read-zeroes": false, "node-name": "disk0" } } diff --git a/tests/qemu-iotests/184.out b/tests/qemu-iotests/184.out index 87c73070e36..ad1762f00f6 100644 --- a/tests/qemu-iotests/184.out +++ b/tests/qemu-iotests/184.out @@ -29,12 +29,12 @@ Testing: "image": { "backing-image": { "virtual-size": 1073741824, - "filename": "null-co://", + "filename": "json:{\"read-zeroes\": false, \"driver\": \"null-co\"}", "format": "null-co", "actual-size": 0 }, "virtual-size": 1073741824, - "filename": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"driver\": \"null-co\"}}", + "filename": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"read-zeroes\": false, \"driver\": \"null-co\"}}", "format": "throttle", "actual-size": 0 }, @@ -54,7 +54,7 @@ Testing: "direct": false, "writeback": true }, - "file": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"driver\": \"null-co\"}}", + "file": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"read-zeroes\": false, \"driver\": \"null-co\"}}", "encryption_key_missing": false }, { @@ -62,7 +62,7 @@ Testing: "detect_zeroes": "off", "image": { "virtual-size": 1073741824, - "filename": "null-co://", + "filename": "json:{\"read-zeroes\": false, \"driver\": \"null-co\"}", "format": "null-co", "actual-size": 0 }, @@ -82,7 +82,7 @@ Testing: "direct": false, "writeback": true }, - "file": "null-co://", + "file": "json:{\"read-zeroes\": false, \"driver\": \"null-co\"}", "encryption_key_missing": false } ] diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218 index ae7c4fb187e..23f8dcc7813 100755 --- a/tests/qemu-iotests/218 +++ b/tests/qemu-iotests/218 @@ -44,12 +44,14 @@ def start_mirror(vm, speed=None, buf_size=None): ret = vm.qmp('blockdev-add', node_name='source', driver='null-co', + read_zeroes=False, size=1048576) assert ret['return'] == {} ret = vm.qmp('blockdev-add', node_name='target', driver='null-co', + read_zeroes=False, size=1048576) assert ret['return'] == {} @@ -174,6 +176,7 @@ with iotests.VM() as vm, \ ret = vm.qmp('blockdev-add', node_name='target', driver='null-co', + read_zeroes=False, size=(64 * 1048576)) assert ret['return'] == {} diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224 index 38dd1536254..4ff8a8e05e6 100755 --- a/tests/qemu-iotests/224 +++ b/tests/qemu-iotests/224 @@ -83,7 +83,8 @@ for filter_node_name in False, True: 'filename': base_img_path }, 'backing': { - 'driver': 'null-co' + 'driver': 'null-co', + 'read-zeroes': False } } }, diff --git a/tests/qemu-iotests/224.out b/tests/qemu-iotests/224.out index 23374a1d295..89e972e30ec 100644 --- a/tests/qemu-iotests/224.out +++ b/tests/qemu-iotests/224.out @@ -1,18 +1,18 @@ --- filter_node_name: False --- -{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}} +{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co", "read-zeroes": false}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}} {"return": {}} -{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}} +{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}} {"return": {}} {"execute": "job-pause", "arguments": {"id": "commit"}} {"return": {}} --- filter_node_name: True --- -{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}} +{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"driver": "null-co", "read-zeroes": false}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}, "node-name": "base"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}, "node-name": "mid"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}, "node-name": "top"}} {"return": {}} -{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "filter-node-name": "filter_node", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}} +{"execute": "block-commit", "arguments": {"base": "json:{\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}", "device": "top", "filter-node-name": "filter_node", "job-id": "commit", "speed": 1, "top": "json:{\"backing\": {\"backing\": {\"read-zeroes\": false, \"driver\": \"null-co\"}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-base.img\"}}, \"driver\": \"IMGFMT\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/PID-mid.img\"}}"}} {"return": {}} {"execute": "job-pause", "arguments": {"id": "commit"}} {"return": {}} diff --git a/tests/qemu-iotests/225 b/tests/qemu-iotests/225 index c0053790db8..f62a1af1ccc 100755 --- a/tests/qemu-iotests/225 +++ b/tests/qemu-iotests/225 @@ -76,7 +76,7 @@ overlay_opts=$(make_opts overlay "$TEST_IMG" backing) base_opts=$(make_opts backing "$TEST_IMG.base") not_base_opts=$(make_opts backing "$TEST_IMG.not_base") -not_vmdk_opts="{ 'node-name': 'backing', 'driver': 'null-co' }" +not_vmdk_opts="{ 'node-name': 'backing', 'driver': 'null-co', 'read-zeroes': false }" echo echo '=== Testing fitting VMDK backing image ===' diff --git a/tests/qemu-iotests/227 b/tests/qemu-iotests/227 index 7e45a47ac61..45955f61855 100755 --- a/tests/qemu-iotests/227 +++ b/tests/qemu-iotests/227 @@ -68,7 +68,7 @@ echo echo '=== blockstats with -drive if=none ===' echo -run_qemu -drive driver=null-co,if=none <<EOF +run_qemu -drive driver=null-co,read-zeroes=off,if=none <<EOF { "execute": "qmp_capabilities" } { "execute": "query-blockstats"} { "execute": "quit" } @@ -78,7 +78,7 @@ echo echo '=== blockstats with -blockdev ===' echo -run_qemu -blockdev driver=null-co,node-name=null <<EOF +run_qemu -blockdev driver=null-co,read-zeroes=off,node-name=null <<EOF { "execute": "qmp_capabilities" } { "execute": "query-blockstats"} { "execute": "quit" } diff --git a/tests/qemu-iotests/227.out b/tests/qemu-iotests/227.out index 9c09ee3917b..f5283b86ce8 100644 --- a/tests/qemu-iotests/227.out +++ b/tests/qemu-iotests/227.out @@ -67,7 +67,7 @@ Testing: -drive driver=null-co,read-zeroes=on,if=virtio === blockstats with -drive if=none === -Testing: -drive driver=null-co,if=none +Testing: -drive driver=null-co,read-zeroes=off,if=none { QMP_VERSION } @@ -131,7 +131,7 @@ Testing: -drive driver=null-co,if=none === blockstats with -blockdev === -Testing: -blockdev driver=null-co,node-name=null +Testing: -blockdev driver=null-co,read-zeroes=off,node-name=null { QMP_VERSION } diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228 index a5eda2e149b..7188b1b00ca 100755 --- a/tests/qemu-iotests/228 +++ b/tests/qemu-iotests/228 @@ -177,7 +177,7 @@ with iotests.FilePath('base.img') as base_img_path, \ # You can only reliably override backing options by using a node # reference (or by specifying file.filename, but, well...) - vm.qmp_log('blockdev-add', node_name='null', driver='null-co') + vm.qmp_log('blockdev-add', node_name='null', driver='null-co', read_zeroes=False) vm.qmp_log('blockdev-add', node_name='node0', diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235 index 20d16dbf38f..6b0b14590b8 100755 --- a/tests/qemu-iotests/235 +++ b/tests/qemu-iotests/235 @@ -65,6 +65,7 @@ log(vm.qmp('blockdev-add', 'throttle-group': 'tg0', 'file': { 'driver': 'null-co', + 'read-zeroes': False, 'size': size } })) diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index cfdeb902be3..f152a7a4e0a 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -602,7 +602,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): ################## ###### null ###### ################## - opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024} + opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024} result = self.vm.qmp('blockdev-add', conv_keys = False, **opts) self.assert_qmp(result, 'return', {}) diff --git a/tests/qemu-iotests/270 b/tests/qemu-iotests/270 index 74352342db5..4aeb545a900 100755 --- a/tests/qemu-iotests/270 +++ b/tests/qemu-iotests/270 @@ -60,7 +60,7 @@ _make_test_img -o cluster_size=2M,data_file="$TEST_IMG.orig" \ # "write" 2G of data without using any space. # (qemu-img create does not like it, though, because null-co does not # support image creation.) -$QEMU_IMG amend -o data_file="json:{'driver':'null-co',,'size':'4294967296'}" \ +$QEMU_IMG amend -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}" \ "$TEST_IMG" # This gives us a range of: diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283 index 79643e375b7..cd235f9ad5f 100755 --- a/tests/qemu-iotests/283 +++ b/tests/qemu-iotests/283 @@ -78,13 +78,14 @@ vm.launch() vm.qmp_log('blockdev-add', **{ 'node-name': 'target', 'driver': 'null-co', + 'read-zeroes': False, 'size': size, }) vm.qmp_log('blockdev-add', **{ 'node-name': 'source', 'driver': 'blkdebug', - 'image': {'node-name': 'base', 'driver': 'null-co', 'size': size} + 'image': {'node-name': 'base', 'driver': 'null-co', 'read-zeroes': False, 'size': size} }) vm.qmp_log('blockdev-add', **{ diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index d8cff22cc19..d5f759a4d91 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -1,6 +1,6 @@ -{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target", "size": 1048576}} +{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target", "read-zeroes": false, "size": 1048576}} {"return": {}} -{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}} +{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "read-zeroes": false, "size": 1048576}, "node-name": "source"}} {"return": {}} {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} {"return": {}} diff --git a/tests/qemu-iotests/299 b/tests/qemu-iotests/299 index a7122941fd2..6fd8402e8f9 100755 --- a/tests/qemu-iotests/299 +++ b/tests/qemu-iotests/299 @@ -36,6 +36,7 @@ vm.launch() vm.qmp_log('blockdev-add', **{ 'node-name': 'disk', 'driver': 'null-co', + 'read-zeroes': False, 'size': 1024 * 1024, }) diff --git a/tests/qemu-iotests/299.out b/tests/qemu-iotests/299.out index bba4252923c..a6f2086e0eb 100644 --- a/tests/qemu-iotests/299.out +++ b/tests/qemu-iotests/299.out @@ -1,4 +1,4 @@ -{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "disk", "size": 1048576}} +{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "disk", "read-zeroes": false, "size": 1048576}} {"return": {}} {"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "disk"}} {"return": {}} diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300 index 43264d883d7..5464c3a9ff7 100755 --- a/tests/qemu-iotests/300 +++ b/tests/qemu-iotests/300 @@ -44,12 +44,12 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): def setUp(self) -> None: self.vm_a = iotests.VM(path_suffix='-a') self.vm_a.add_blockdev(f'node-name={self.src_node_name},' - 'driver=null-co') + 'driver=null-co,read-zeroes=off') self.vm_a.launch() self.vm_b = iotests.VM(path_suffix='-b') self.vm_b.add_blockdev(f'node-name={self.dst_node_name},' - 'driver=null-co') + 'driver=null-co,read-zeroes=off') self.vm_b.add_incoming(f'unix:{mig_sock}') self.vm_b.launch() -- 2.26.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' 2021-02-11 14:26 ` [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' Philippe Mathieu-Daudé @ 2021-02-11 16:29 ` Vladimir Sementsov-Ogievskiy 2021-02-11 19:19 ` Philippe Mathieu-Daudé 2021-02-11 22:40 ` Eric Blake 2021-02-12 19:06 ` Eric Blake 2 siblings, 1 reply; 24+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-02-11 16:29 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf 11.02.2021 17:26, Philippe Mathieu-Daudé wrote: > We are going to switch the 'null-co' default 'read-zeroes' value > from FALSE to TRUE in the next commit. First explicit the FALSE > value when it is not set. > > Suggested-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > - Missing: 056 & 155. I couldn't figure out the proper syntax, > any help welcomed... > - I'm unsure about 162, this doesn't seem to use the null-co > driver but rather testing global syntax. > --- > docs/devel/testing.rst | 14 +++++++------- > tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- > tests/test-bdrv-drain.c | 10 ++++++++-- > tests/acceptance/virtio_check_params.py | 2 +- > tests/perf/block/qcow2/convert-blockstatus | 6 +++--- > tests/qemu-iotests/040 | 2 +- > tests/qemu-iotests/041 | 12 ++++++++---- > tests/qemu-iotests/051 | 2 +- > tests/qemu-iotests/051.out | 2 +- > tests/qemu-iotests/051.pc.out | 4 ++-- > tests/qemu-iotests/087 | 6 ++++-- > tests/qemu-iotests/118 | 2 +- > tests/qemu-iotests/133 | 2 +- > tests/qemu-iotests/153 | 8 ++++---- > tests/qemu-iotests/184 | 2 ++ > tests/qemu-iotests/184.out | 10 +++++----- > tests/qemu-iotests/218 | 3 +++ > tests/qemu-iotests/224 | 3 ++- > tests/qemu-iotests/224.out | 8 ++++---- > tests/qemu-iotests/225 | 2 +- > tests/qemu-iotests/227 | 4 ++-- > tests/qemu-iotests/227.out | 4 ++-- > tests/qemu-iotests/228 | 2 +- > tests/qemu-iotests/235 | 1 + > tests/qemu-iotests/245 | 2 +- > tests/qemu-iotests/270 | 2 +- > tests/qemu-iotests/283 | 3 ++- > tests/qemu-iotests/283.out | 4 ++-- > tests/qemu-iotests/299 | 1 + > tests/qemu-iotests/299.out | 2 +- > tests/qemu-iotests/300 | 4 ++-- Why do you think these tests will work bad with new default? At least everything under tests/qemu-iotests/ and tests/test-bdrv-drain are not about performance tests/perf/block/qcow2/convert-blockstatus is OK with new default too. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' 2021-02-11 16:29 ` Vladimir Sementsov-Ogievskiy @ 2021-02-11 19:19 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-11 19:19 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf Hi Vladimir, On 2/11/21 5:29 PM, Vladimir Sementsov-Ogievskiy wrote: > 11.02.2021 17:26, Philippe Mathieu-Daudé wrote: >> We are going to switch the 'null-co' default 'read-zeroes' value >> from FALSE to TRUE in the next commit. First explicit the FALSE >> value when it is not set. >> >> Suggested-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> - Missing: 056 & 155. I couldn't figure out the proper syntax, >> any help welcomed... >> - I'm unsure about 162, this doesn't seem to use the null-co >> driver but rather testing global syntax. >> --- >> docs/devel/testing.rst | 14 +++++++------- >> tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- >> tests/test-bdrv-drain.c | 10 ++++++++-- >> tests/acceptance/virtio_check_params.py | 2 +- >> tests/perf/block/qcow2/convert-blockstatus | 6 +++--- >> tests/qemu-iotests/040 | 2 +- >> tests/qemu-iotests/041 | 12 ++++++++---- >> tests/qemu-iotests/051 | 2 +- >> tests/qemu-iotests/051.out | 2 +- >> tests/qemu-iotests/051.pc.out | 4 ++-- >> tests/qemu-iotests/087 | 6 ++++-- >> tests/qemu-iotests/118 | 2 +- >> tests/qemu-iotests/133 | 2 +- >> tests/qemu-iotests/153 | 8 ++++---- >> tests/qemu-iotests/184 | 2 ++ >> tests/qemu-iotests/184.out | 10 +++++----- >> tests/qemu-iotests/218 | 3 +++ >> tests/qemu-iotests/224 | 3 ++- >> tests/qemu-iotests/224.out | 8 ++++---- >> tests/qemu-iotests/225 | 2 +- >> tests/qemu-iotests/227 | 4 ++-- >> tests/qemu-iotests/227.out | 4 ++-- >> tests/qemu-iotests/228 | 2 +- >> tests/qemu-iotests/235 | 1 + >> tests/qemu-iotests/245 | 2 +- >> tests/qemu-iotests/270 | 2 +- >> tests/qemu-iotests/283 | 3 ++- >> tests/qemu-iotests/283.out | 4 ++-- >> tests/qemu-iotests/299 | 1 + >> tests/qemu-iotests/299.out | 2 +- >> tests/qemu-iotests/300 | 4 ++-- > > Why do you think these tests will work bad with new default? As I don't understand the tests, this was the deal with Eric :) "OK to change default if current default is explicited" then block maintainers could audit each case and see which one can safely use read-zeroes=true. > > At least everything under tests/qemu-iotests/ and tests/test-bdrv-drain > are not about performance > > tests/perf/block/qcow2/convert-blockstatus is OK with new default too. OK, I'll see who should send these patches on top with Eric & Max. Thank for your review, Phil. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' 2021-02-11 14:26 ` [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' Philippe Mathieu-Daudé 2021-02-11 16:29 ` Vladimir Sementsov-Ogievskiy @ 2021-02-11 22:40 ` Eric Blake 2021-02-11 23:49 ` Philippe Mathieu-Daudé 2021-02-12 11:34 ` Vladimir Sementsov-Ogievskiy 2021-02-12 19:06 ` Eric Blake 2 siblings, 2 replies; 24+ messages in thread From: Eric Blake @ 2021-02-11 22:40 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote: > We are going to switch the 'null-co' default 'read-zeroes' value > from FALSE to TRUE in the next commit. First explicit the FALSE > value when it is not set. Grammar suggestion, along with a suggestion for an additional sentence to make the intent of this commit clearer: As a first step, request an explicit FALSE value rather than relying on the defaults. This is intended to be a purely mechanical adjustment for no performance behavior in the tests; later patches may then flip or elide the explicit choice for tests where performance does not matter. > > Suggested-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > - Missing: 056 & 155. I couldn't figure out the proper syntax, > any help welcomed... 056 - looks like just one line: self.vm = iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug") the best way to add it here would be rewriting that line to use blockdev syntax rather than blkdebug: URI syntax. The other question is whether it is a noticeable time difference when the default is flipped in 2/2. 155 - looks like several uses such as: class TestBlockdevMirrorForcedBacking(MirrorBaseClass): cmd = 'blockdev-mirror' existing = True target_backing = None target_blockdev_backing = { 'driver': 'null-co' } target_real_backing = 'null-co://' > - I'm unsure about 162, this doesn't seem to use the null-co > driver but rather testing global syntax. Concur; I don't see any reason to worry about this one (but mentioning it in the commit message can't hurt in case someone asks later) # blkdebug expects all of its arguments to be strings, but its # bdrv_refresh_filename() implementation should not assume that they have been # passed as strings in the original options QDict. # So this should emit blkdebug:42:null-co:// as the filename: touch 42 $QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42, "image.driver": "null-co"}' \ > --- > docs/devel/testing.rst | 14 +++++++------- > tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- > tests/test-bdrv-drain.c | 10 ++++++++-- > tests/acceptance/virtio_check_params.py | 2 +- > tests/perf/block/qcow2/convert-blockstatus | 6 +++--- > tests/qemu-iotests/040 | 2 +- You did a pretty good hunt for culprits! > tests/qemu-iotests/041 | 12 ++++++++---- > tests/qemu-iotests/051 | 2 +- > tests/qemu-iotests/051.out | 2 +- > tests/qemu-iotests/051.pc.out | 4 ++-- and for the fallout to the iotests. I did not audit for which tests are easy candidates for dropping the explicit read-zeroes=false (that is, where the extra time in allowing the flipped default doesn't penalize the test), but am okay giving this patch: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' 2021-02-11 22:40 ` Eric Blake @ 2021-02-11 23:49 ` Philippe Mathieu-Daudé 2021-02-12 11:34 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-11 23:49 UTC (permalink / raw) To: Eric Blake, qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf On 2/11/21 11:40 PM, Eric Blake wrote: > On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote: >> We are going to switch the 'null-co' default 'read-zeroes' value >> from FALSE to TRUE in the next commit. First explicit the FALSE >> value when it is not set. > > Grammar suggestion, along with a suggestion for an additional sentence > to make the intent of this commit clearer: > > As a first step, request an explicit FALSE value rather than relying on > the defaults. This is intended to be a purely mechanical adjustment for > no performance behavior in the tests; later patches may then flip or > elide the explicit choice for tests where performance does not matter. > >> >> Suggested-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> - Missing: 056 & 155. I couldn't figure out the proper syntax, >> any help welcomed... > > 056 - looks like just one line: > self.vm = > iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug") > > the best way to add it here would be rewriting that line to use blockdev > syntax rather than blkdebug: URI syntax. The other question is whether > it is a noticeable time difference when the default is flipped in 2/2. > > 155 - looks like several uses such as: > > class TestBlockdevMirrorForcedBacking(MirrorBaseClass): > cmd = 'blockdev-mirror' > existing = True > target_backing = None > target_blockdev_backing = { 'driver': 'null-co' } > target_real_backing = 'null-co://' > > >> - I'm unsure about 162, this doesn't seem to use the null-co >> driver but rather testing global syntax. > > Concur; I don't see any reason to worry about this one (but mentioning > it in the commit message can't hurt in case someone asks later) > > # blkdebug expects all of its arguments to be strings, but its > # bdrv_refresh_filename() implementation should not assume that they > have been > # passed as strings in the original options QDict. > # So this should emit blkdebug:42:null-co:// as the filename: > touch 42 > $QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42, > "image.driver": "null-co"}' \ > > >> --- >> docs/devel/testing.rst | 14 +++++++------- >> tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- >> tests/test-bdrv-drain.c | 10 ++++++++-- >> tests/acceptance/virtio_check_params.py | 2 +- >> tests/perf/block/qcow2/convert-blockstatus | 6 +++--- >> tests/qemu-iotests/040 | 2 +- > > You did a pretty good hunt for culprits! > >> tests/qemu-iotests/041 | 12 ++++++++---- >> tests/qemu-iotests/051 | 2 +- >> tests/qemu-iotests/051.out | 2 +- >> tests/qemu-iotests/051.pc.out | 4 ++-- > > and for the fallout to the iotests. > > I did not audit for which tests are easy candidates for dropping the > explicit read-zeroes=false (that is, where the extra time in allowing > the flipped default doesn't penalize the test), but am okay giving this > patch: > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks for your help. I'll address your comments and respin. Regards, Phil. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' 2021-02-11 22:40 ` Eric Blake 2021-02-11 23:49 ` Philippe Mathieu-Daudé @ 2021-02-12 11:34 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 24+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-02-12 11:34 UTC (permalink / raw) To: Eric Blake, Philippe Mathieu-Daudé, qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf 12.02.2021 01:40, Eric Blake wrote: > On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote: >> We are going to switch the 'null-co' default 'read-zeroes' value >> from FALSE to TRUE in the next commit. First explicit the FALSE >> value when it is not set. > > Grammar suggestion, along with a suggestion for an additional sentence > to make the intent of this commit clearer: > > As a first step, request an explicit FALSE value rather than relying on > the defaults. This is intended to be a purely mechanical adjustment for > no performance behavior in the tests; later patches may then flip or > elide the explicit choice for tests where performance does not matter. > >> >> Suggested-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> - Missing: 056 & 155. I couldn't figure out the proper syntax, >> any help welcomed... > > 056 - looks like just one line: > self.vm = > iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug") > > the best way to add it here would be rewriting that line to use blockdev > syntax rather than blkdebug: URI syntax. The other question is whether > it is a noticeable time difference when the default is flipped in 2/2. > > 155 - looks like several uses such as: > > class TestBlockdevMirrorForcedBacking(MirrorBaseClass): > cmd = 'blockdev-mirror' > existing = True > target_backing = None > target_blockdev_backing = { 'driver': 'null-co' } > target_real_backing = 'null-co://' > > >> - I'm unsure about 162, this doesn't seem to use the null-co >> driver but rather testing global syntax. > > Concur; I don't see any reason to worry about this one (but mentioning > it in the commit message can't hurt in case someone asks later) > > # blkdebug expects all of its arguments to be strings, but its > # bdrv_refresh_filename() implementation should not assume that they > have been > # passed as strings in the original options QDict. > # So this should emit blkdebug:42:null-co:// as the filename: > touch 42 > $QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42, > "image.driver": "null-co"}' \ > > >> --- >> docs/devel/testing.rst | 14 +++++++------- >> tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- >> tests/test-bdrv-drain.c | 10 ++++++++-- >> tests/acceptance/virtio_check_params.py | 2 +- >> tests/perf/block/qcow2/convert-blockstatus | 6 +++--- >> tests/qemu-iotests/040 | 2 +- > > You did a pretty good hunt for culprits! > >> tests/qemu-iotests/041 | 12 ++++++++---- >> tests/qemu-iotests/051 | 2 +- >> tests/qemu-iotests/051.out | 2 +- >> tests/qemu-iotests/051.pc.out | 4 ++-- > > and for the fallout to the iotests. > > I did not audit for which tests are easy candidates for dropping the > explicit read-zeroes=false (that is, where the extra time in allowing > the flipped default doesn't penalize the test), but am okay giving this > patch: > > Reviewed-by: Eric Blake <eblake@redhat.com> > > I'd keep tests/qemu-iotests as is.. Benefits of keeping old behavior (by adding read-zeroes=false) - saving several milliseconds. I doubt that it would be even noticeable. Could you give have much time of "make check-block" it saves, if we update all iotests to use read-zeroes=false, instead of transparently move to new default? Drawbacks of adding read-zeroes=false: - it's generally better to test only default behavior than only non-default behavior - this patch creates extra conflicts on any patch porting between upstream/downstream - for new tests, nobady will care about read-zeroes and will not add it.. (or will add when copy existing test no thinking about it), so it will be a kind of mess -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' 2021-02-11 14:26 ` [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' Philippe Mathieu-Daudé 2021-02-11 16:29 ` Vladimir Sementsov-Ogievskiy 2021-02-11 22:40 ` Eric Blake @ 2021-02-12 19:06 ` Eric Blake 2 siblings, 0 replies; 24+ messages in thread From: Eric Blake @ 2021-02-12 19:06 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote: > We are going to switch the 'null-co' default 'read-zeroes' value > from FALSE to TRUE in the next commit. First explicit the FALSE > value when it is not set. > > Suggested-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > +++ b/tests/qemu-iotests/300 > @@ -44,12 +44,12 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): > def setUp(self) -> None: > self.vm_a = iotests.VM(path_suffix='-a') > self.vm_a.add_blockdev(f'node-name={self.src_node_name},' > - 'driver=null-co') > + 'driver=null-co,read-zeroes=off') > self.vm_a.launch() > > self.vm_b = iotests.VM(path_suffix='-b') > self.vm_b.add_blockdev(f'node-name={self.dst_node_name},' > - 'driver=null-co') > + 'driver=null-co,read-zeroes=off') > self.vm_b.add_incoming(f'unix:{mig_sock}') > self.vm_b.launch() > > Incomplete: 300 has a couple more lines that look like: tests/qemu-iotests/300- result = self.vm_a.qmp('blockdev-add', tests/qemu-iotests/300: node_name='node-b', driver='null-co') -- tests/qemu-iotests/300- result = self.vm_b.qmp('blockdev-add', tests/qemu-iotests/300: node_name='node-a', driver='null-co') that could use the same treatment (noticed while reviewing Peter's patch to add yet more uses in that test). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default 2021-02-11 14:26 [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Philippe Mathieu-Daudé 2021-02-11 14:26 ` [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' Philippe Mathieu-Daudé @ 2021-02-11 14:26 ` Philippe Mathieu-Daudé 2021-02-11 16:22 ` Stefan Hajnoczi 2021-02-11 15:42 ` [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Alexander Bulekov 2021-02-13 21:54 ` Fam Zheng 3 siblings, 1 reply; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-11 14:26 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf The null-co driver is meant for (performance) testing. By default, read operation does nothing, the provided buffer is not filled with zero values and its content is unchanged. This performance 'feature' becomes an issue from a security perspective. For example, using the default null-co driver, buf[] is uninitialized, the blk_pread() call succeeds and we then access uninitialized memory: static int guess_disk_lchs(BlockBackend *blk, int *pcylinders, int *pheads, int *psectors) { uint8_t buf[BDRV_SECTOR_SIZE]; ... if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) { return -1; } /* test msdos magic */ if (buf[510] != 0x55 || buf[511] != 0xaa) { return -1; } We could audit all the uninitialized buffers and the bdrv_co_preadv() handlers, but it is simpler to change the default of this testing driver. Performance tests will have to adapt and use 'null-co,read-zeroes=off'. Suggested-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- block/null.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/null.c b/block/null.c index cc9b1d4ea72..f9658fd70ac 100644 --- a/block/null.c +++ b/block/null.c @@ -93,7 +93,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags, error_setg(errp, "latency-ns is invalid"); ret = -EINVAL; } - s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false); + s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, true); qemu_opts_del(opts); bs->supported_write_flags = BDRV_REQ_FUA; return ret; -- 2.26.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default 2021-02-11 14:26 ` [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default Philippe Mathieu-Daudé @ 2021-02-11 16:22 ` Stefan Hajnoczi 0 siblings, 0 replies; 24+ messages in thread From: Stefan Hajnoczi @ 2021-02-11 16:22 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Alexander Bulekov, Bandan Das, Kevin Wolf, Cleber Rosa, Paolo Bonzini, Max Reitz [-- Attachment #1: Type: text/plain, Size: 1414 bytes --] On Thu, Feb 11, 2021 at 03:26:56PM +0100, Philippe Mathieu-Daudé wrote: > The null-co driver is meant for (performance) testing. > By default, read operation does nothing, the provided buffer > is not filled with zero values and its content is unchanged. > > This performance 'feature' becomes an issue from a security > perspective. For example, using the default null-co driver, > buf[] is uninitialized, the blk_pread() call succeeds and we > then access uninitialized memory: > > static int guess_disk_lchs(BlockBackend *blk, > int *pcylinders, int *pheads, > int *psectors) > { > uint8_t buf[BDRV_SECTOR_SIZE]; > ... > > if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) { > return -1; > } > /* test msdos magic */ > if (buf[510] != 0x55 || buf[511] != 0xaa) { > return -1; > } > > We could audit all the uninitialized buffers and the > bdrv_co_preadv() handlers, but it is simpler to change the > default of this testing driver. Performance tests will have > to adapt and use 'null-co,read-zeroes=off'. > > Suggested-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > block/null.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-11 14:26 [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Philippe Mathieu-Daudé 2021-02-11 14:26 ` [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' Philippe Mathieu-Daudé 2021-02-11 14:26 ` [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default Philippe Mathieu-Daudé @ 2021-02-11 15:42 ` Alexander Bulekov 2021-02-12 14:32 ` Philippe Mathieu-Daudé 2021-02-13 21:54 ` Fam Zheng 3 siblings, 1 reply; 24+ messages in thread From: Alexander Bulekov @ 2021-02-11 15:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Kevin Wolf, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Max Reitz On 210211 1526, Philippe Mathieu-Daudé wrote: > The null-co driver doesn't zeroize buffer in its default config, > because it is designed for testing and tests want to run fast. > However this confuses security researchers (access to uninit > buffers). > Interesting.. Is there an example bug report, where it raised alarms because of an un-zeroed null-co:// buffer? -Alex > A one-line patch supposed which became a painful one, because > there is so many different syntax to express the same usage: > > opt = qdict_new(); > qdict_put_str(opt, "read-zeroes", "off"); > null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL, > &error_abort); > > vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...) > > vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none") > > blk0 = { 'node-name': 'src', > 'driver': 'null-co', > 'read-zeroes': 'off' } > > 'file': { > 'driver': 'null-co', > 'read-zeroes': False, > } > > "file": { > "driver": "null-co", > "read-zeroes": "off" > } > > { "execute": "blockdev-add", > "arguments": { > "driver": "null-co", > "read-zeroes": false, > "node-name": "disk0" > } > } > > opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024} > > qemu -drive driver=null-co,read-zeroes=off > > qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}" > > qemu-img null-co://,read-zeroes=off > > qemu-img ... -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}" > > There are probably more. > > Anyhow, the iotests I am not sure and should be audited are 056, 155 > (I don't understand the syntax) and 162. > > Please review, > > Phil. > > Philippe Mathieu-Daud=C3=A9 (2): > block: Explicit null-co uses 'read-zeroes=3Dfalse' > block/null: Enable 'read-zeroes' mode by default > > docs/devel/testing.rst | 14 +++++++------- > tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- > block/null.c | 2 +- > tests/test-bdrv-drain.c | 10 ++++++++-- > tests/acceptance/virtio_check_params.py | 2 +- > tests/perf/block/qcow2/convert-blockstatus | 6 +++--- > tests/qemu-iotests/040 | 2 +- > tests/qemu-iotests/041 | 12 ++++++++---- > tests/qemu-iotests/051 | 2 +- > tests/qemu-iotests/051.out | 2 +- > tests/qemu-iotests/051.pc.out | 4 ++-- > tests/qemu-iotests/087 | 6 ++++-- > tests/qemu-iotests/118 | 2 +- > tests/qemu-iotests/133 | 2 +- > tests/qemu-iotests/153 | 8 ++++---- > tests/qemu-iotests/184 | 2 ++ > tests/qemu-iotests/184.out | 10 +++++----- > tests/qemu-iotests/218 | 3 +++ > tests/qemu-iotests/224 | 3 ++- > tests/qemu-iotests/224.out | 8 ++++---- > tests/qemu-iotests/225 | 2 +- > tests/qemu-iotests/227 | 4 ++-- > tests/qemu-iotests/227.out | 4 ++-- > tests/qemu-iotests/228 | 2 +- > tests/qemu-iotests/235 | 1 + > tests/qemu-iotests/245 | 2 +- > tests/qemu-iotests/270 | 2 +- > tests/qemu-iotests/283 | 3 ++- > tests/qemu-iotests/283.out | 4 ++-- > tests/qemu-iotests/299 | 1 + > tests/qemu-iotests/299.out | 2 +- > tests/qemu-iotests/300 | 4 ++-- > 32 files changed, 82 insertions(+), 60 deletions(-) > > --=20 > 2.26.2 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-11 15:42 ` [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Alexander Bulekov @ 2021-02-12 14:32 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-12 14:32 UTC (permalink / raw) To: Alexander Bulekov Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Kevin Wolf, Bandan Das, Andrey Shinkevich, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Max Reitz On 2/11/21 4:42 PM, Alexander Bulekov wrote: > On 210211 1526, Philippe Mathieu-Daudé wrote: >> The null-co driver doesn't zeroize buffer in its default config, >> because it is designed for testing and tests want to run fast. >> However this confuses security researchers (access to uninit >> buffers). >> > > Interesting.. Is there an example bug report, where it raised alarms > because of an un-zeroed null-co:// buffer? No, but I found a similar mention here: https://www.mail-archive.com/qemu-block@nongnu.org/msg52045.html Example: $ valgrind qemu-system-i386 -S -drive file=null-co://,format=raw,file.read-zeroes=on $ valgrind qemu-system-i386 -S -drive file=null-co://,format=raw,file.read-zeroes=off ==4048219== Conditional jump or move depends on uninitialised value(s) ==4048219== at 0x4E19CC: guess_disk_lchs (hd-geometry.c:70) ==4048219== by 0x4E1C72: hd_geometry_guess (hd-geometry.c:131) ==4048219== by 0x4E0F0F: blkconf_geometry (block.c:183) ==4048219== by 0x563727: ide_dev_initfn (qdev.c:201) ==4048219== by 0x563AE4: ide_hd_realize (qdev.c:278) ==4048219== by 0x563320: ide_qdev_realize (qdev.c:124) ==4048219== by 0x8F8EAA: device_set_realized (qdev.c:761) ==4048219== by 0x902347: property_set_bool (object.c:2255) ==4048219== by 0x900441: object_property_set (object.c:1400) ==4048219== by 0x904467: object_property_set_qobject (qom-qobject.c:28) ==4048219== by 0x9007A4: object_property_set_bool (object.c:1470) ==4048219== by 0x8F7F3B: qdev_realize (qdev.c:389) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-11 14:26 [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2021-02-11 15:42 ` [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Alexander Bulekov @ 2021-02-13 21:54 ` Fam Zheng 2021-02-19 11:07 ` Max Reitz 3 siblings, 1 reply; 24+ messages in thread From: Fam Zheng @ 2021-02-13 21:54 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Max Reitz On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: > The null-co driver doesn't zeroize buffer in its default config, > because it is designed for testing and tests want to run fast. > However this confuses security researchers (access to uninit > buffers). I'm a little surprised. Is changing default the only way to fix this? I'm not opposed to changing the default but I'm not convinced this is the easiest way. block/nvme.c also doesn't touch the memory, but defers to the device DMA, why doesn't that confuse the security checker? Cannot we just somehow annotate it in a way that the checker can understand (akin to how we provide coverity models) and be done? Thanks, Fam > > A one-line patch supposed which became a painful one, because > there is so many different syntax to express the same usage: > > opt = qdict_new(); > qdict_put_str(opt, "read-zeroes", "off"); > null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL, > &error_abort); > > vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...) > > vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none") > > blk0 = { 'node-name': 'src', > 'driver': 'null-co', > 'read-zeroes': 'off' } > > 'file': { > 'driver': 'null-co', > 'read-zeroes': False, > } > > "file": { > "driver": "null-co", > "read-zeroes": "off" > } > > { "execute": "blockdev-add", > "arguments": { > "driver": "null-co", > "read-zeroes": false, > "node-name": "disk0" > } > } > > opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024} > > qemu -drive driver=null-co,read-zeroes=off > > qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}" > > qemu-img null-co://,read-zeroes=off > > qemu-img ... -o data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}" > > There are probably more. > > Anyhow, the iotests I am not sure and should be audited are 056, 155 > (I don't understand the syntax) and 162. > > Please review, > > Phil. > > Philippe Mathieu-Daud=C3=A9 (2): > block: Explicit null-co uses 'read-zeroes=3Dfalse' > block/null: Enable 'read-zeroes' mode by default > > docs/devel/testing.rst | 14 +++++++------- > tests/qtest/fuzz/generic_fuzz_configs.h | 11 ++++++----- > block/null.c | 2 +- > tests/test-bdrv-drain.c | 10 ++++++++-- > tests/acceptance/virtio_check_params.py | 2 +- > tests/perf/block/qcow2/convert-blockstatus | 6 +++--- > tests/qemu-iotests/040 | 2 +- > tests/qemu-iotests/041 | 12 ++++++++---- > tests/qemu-iotests/051 | 2 +- > tests/qemu-iotests/051.out | 2 +- > tests/qemu-iotests/051.pc.out | 4 ++-- > tests/qemu-iotests/087 | 6 ++++-- > tests/qemu-iotests/118 | 2 +- > tests/qemu-iotests/133 | 2 +- > tests/qemu-iotests/153 | 8 ++++---- > tests/qemu-iotests/184 | 2 ++ > tests/qemu-iotests/184.out | 10 +++++----- > tests/qemu-iotests/218 | 3 +++ > tests/qemu-iotests/224 | 3 ++- > tests/qemu-iotests/224.out | 8 ++++---- > tests/qemu-iotests/225 | 2 +- > tests/qemu-iotests/227 | 4 ++-- > tests/qemu-iotests/227.out | 4 ++-- > tests/qemu-iotests/228 | 2 +- > tests/qemu-iotests/235 | 1 + > tests/qemu-iotests/245 | 2 +- > tests/qemu-iotests/270 | 2 +- > tests/qemu-iotests/283 | 3 ++- > tests/qemu-iotests/283.out | 4 ++-- > tests/qemu-iotests/299 | 1 + > tests/qemu-iotests/299.out | 2 +- > tests/qemu-iotests/300 | 4 ++-- > 32 files changed, 82 insertions(+), 60 deletions(-) > > --=20 > 2.26.2 > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-13 21:54 ` Fam Zheng @ 2021-02-19 11:07 ` Max Reitz 2021-02-19 14:09 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 24+ messages in thread From: Max Reitz @ 2021-02-19 11:07 UTC (permalink / raw) To: Fam Zheng, Philippe Mathieu-Daudé Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini On 13.02.21 22:54, Fam Zheng wrote: > On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: >> The null-co driver doesn't zeroize buffer in its default config, >> because it is designed for testing and tests want to run fast. >> However this confuses security researchers (access to uninit >> buffers). > > I'm a little surprised. > > Is changing default the only way to fix this? I'm not opposed to > changing the default but I'm not convinced this is the easiest way. > block/nvme.c also doesn't touch the memory, but defers to the device > DMA, why doesn't that confuse the security checker? > > Cannot we just somehow annotate it in a way that the checker can > understand (akin to how we provide coverity models) and be done? The question is, why wouldn’t we change the default? read-zeroes=true seems the better default to me. I consider silencing valgrind warnings and the like a nice side effect. Max ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-19 11:07 ` Max Reitz @ 2021-02-19 14:09 ` Philippe Mathieu-Daudé 2021-02-22 17:35 ` Fam Zheng 2021-02-22 18:15 ` Daniel P. Berrangé 0 siblings, 2 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-19 14:09 UTC (permalink / raw) To: Max Reitz, Fam Zheng Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini On 2/19/21 12:07 PM, Max Reitz wrote: > On 13.02.21 22:54, Fam Zheng wrote: >> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: >>> The null-co driver doesn't zeroize buffer in its default config, >>> because it is designed for testing and tests want to run fast. >>> However this confuses security researchers (access to uninit >>> buffers). >> >> I'm a little surprised. >> >> Is changing default the only way to fix this? I'm not opposed to >> changing the default but I'm not convinced this is the easiest way. >> block/nvme.c also doesn't touch the memory, but defers to the device >> DMA, why doesn't that confuse the security checker? Generally speaking, there is a balance between security and performance. We try to provide both, but when we can't, my understanding is security is more important. Customers expect a secure product. If they prefer performance and at the price of security, it is also possible by enabling an option that is not the default. I'm not sure why you mention block/nvme here. I have the understanding the null-co driver is only useful for testing. Are there production cases where null-co is used? BTW block/nvme is a particular driver where performance matters more than security (but we have to make sure the users are aware of that). >> Cannot we just somehow annotate it in a way that the checker can >> understand (akin to how we provide coverity models) and be done? > > The question is, why wouldn’t we change the default? read-zeroes=true > seems the better default to me. I consider silencing valgrind warnings > and the like a nice side effect. > > Max > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-19 14:09 ` Philippe Mathieu-Daudé @ 2021-02-22 17:35 ` Fam Zheng 2021-02-22 17:55 ` Philippe Mathieu-Daudé 2021-02-22 18:15 ` Daniel P. Berrangé 1 sibling, 1 reply; 24+ messages in thread From: Fam Zheng @ 2021-02-22 17:35 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, Markus Armbruster, qemu-devel, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote: > On 2/19/21 12:07 PM, Max Reitz wrote: > > On 13.02.21 22:54, Fam Zheng wrote: > >> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: > >>> The null-co driver doesn't zeroize buffer in its default config, > >>> because it is designed for testing and tests want to run fast. > >>> However this confuses security researchers (access to uninit > >>> buffers). > >> > >> I'm a little surprised. > >> > >> Is changing default the only way to fix this? I'm not opposed to > >> changing the default but I'm not convinced this is the easiest way. > >> block/nvme.c also doesn't touch the memory, but defers to the device > >> DMA, why doesn't that confuse the security checker? > > Generally speaking, there is a balance between security and performance. > We try to provide both, but when we can't, my understanding is security > is more important. Why is hiding the code path behind a non-default more secure? What is not secure now? Fam ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-22 17:35 ` Fam Zheng @ 2021-02-22 17:55 ` Philippe Mathieu-Daudé 2021-02-23 9:21 ` Fam Zheng 0 siblings, 1 reply; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-22 17:55 UTC (permalink / raw) To: Fam Zheng Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, Markus Armbruster, qemu-devel, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini On 2/22/21 6:35 PM, Fam Zheng wrote: > On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote: >> On 2/19/21 12:07 PM, Max Reitz wrote: >>> On 13.02.21 22:54, Fam Zheng wrote: >>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: >>>>> The null-co driver doesn't zeroize buffer in its default config, >>>>> because it is designed for testing and tests want to run fast. >>>>> However this confuses security researchers (access to uninit >>>>> buffers). >>>> >>>> I'm a little surprised. >>>> >>>> Is changing default the only way to fix this? I'm not opposed to >>>> changing the default but I'm not convinced this is the easiest way. >>>> block/nvme.c also doesn't touch the memory, but defers to the device >>>> DMA, why doesn't that confuse the security checker? >> >> Generally speaking, there is a balance between security and performance. >> We try to provide both, but when we can't, my understanding is security >> is more important. > > Why is hiding the code path behind a non-default more secure? What is > not secure now? Se we are back to the problem of having default values. I'd like to remove the default and have the option explicit, but qemu_opt_get_bool() expects a 'default' value. Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default() and add a simpler qemu_opt_get_bool()? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-22 17:55 ` Philippe Mathieu-Daudé @ 2021-02-23 9:21 ` Fam Zheng 2021-02-23 16:01 ` Max Reitz 0 siblings, 1 reply; 24+ messages in thread From: Fam Zheng @ 2021-02-23 9:21 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, qemu-devel, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Max Reitz On 2021-02-22 18:55, Philippe Mathieu-Daudé wrote: > On 2/22/21 6:35 PM, Fam Zheng wrote: > > On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote: > >> On 2/19/21 12:07 PM, Max Reitz wrote: > >>> On 13.02.21 22:54, Fam Zheng wrote: > >>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: > >>>>> The null-co driver doesn't zeroize buffer in its default config, > >>>>> because it is designed for testing and tests want to run fast. > >>>>> However this confuses security researchers (access to uninit > >>>>> buffers). > >>>> > >>>> I'm a little surprised. > >>>> > >>>> Is changing default the only way to fix this? I'm not opposed to > >>>> changing the default but I'm not convinced this is the easiest way. > >>>> block/nvme.c also doesn't touch the memory, but defers to the device > >>>> DMA, why doesn't that confuse the security checker? > >> > >> Generally speaking, there is a balance between security and performance. > >> We try to provide both, but when we can't, my understanding is security > >> is more important. > > > > Why is hiding the code path behind a non-default more secure? What is > > not secure now? > > Se we are back to the problem of having default values. > > I'd like to remove the default and have the option explicit, > but qemu_opt_get_bool() expects a 'default' value. > > Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default() > and add a simpler qemu_opt_get_bool()? My point is I still don't get the full context of the problem this series is trying to solve. If the problem is tools are confused, I would like to understand why. What is the thing that matters here, exactly? But there's always been nullblk.ko in kernel that doesn't lie in the name. If we change the default we are not very honest any more about what the driver actually does. Even if null-co:// and null-aio:// is a bad idea, then let's add zero-co://co and zero-aio://, and deprecate null-*://. Fam ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-23 9:21 ` Fam Zheng @ 2021-02-23 16:01 ` Max Reitz 2021-02-23 17:21 ` Fam Zheng 0 siblings, 1 reply; 24+ messages in thread From: Max Reitz @ 2021-02-23 16:01 UTC (permalink / raw) To: Fam Zheng, Philippe Mathieu-Daudé Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, qemu-devel, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini On 23.02.21 10:21, Fam Zheng wrote: > On 2021-02-22 18:55, Philippe Mathieu-Daudé wrote: >> On 2/22/21 6:35 PM, Fam Zheng wrote: >>> On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote: >>>> On 2/19/21 12:07 PM, Max Reitz wrote: >>>>> On 13.02.21 22:54, Fam Zheng wrote: >>>>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: >>>>>>> The null-co driver doesn't zeroize buffer in its default config, >>>>>>> because it is designed for testing and tests want to run fast. >>>>>>> However this confuses security researchers (access to uninit >>>>>>> buffers). >>>>>> >>>>>> I'm a little surprised. >>>>>> >>>>>> Is changing default the only way to fix this? I'm not opposed to >>>>>> changing the default but I'm not convinced this is the easiest way. >>>>>> block/nvme.c also doesn't touch the memory, but defers to the device >>>>>> DMA, why doesn't that confuse the security checker? >>>> >>>> Generally speaking, there is a balance between security and performance. >>>> We try to provide both, but when we can't, my understanding is security >>>> is more important. >>> >>> Why is hiding the code path behind a non-default more secure? What is >>> not secure now? >> >> Se we are back to the problem of having default values. >> >> I'd like to remove the default and have the option explicit, >> but qemu_opt_get_bool() expects a 'default' value. >> >> Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default() >> and add a simpler qemu_opt_get_bool()? > > My point is I still don't get the full context of the problem this > series is trying to solve. If the problem is tools are confused, I would > like to understand why. What is the thing that matters here, exactly? My personal opinion is that it isn’t even about tools, it’s just about the fact that operating on uninitialized data is something that should generally be avoided. For the null drivers, there is a reason to allow it (performance testing), but that should be a special case, not the default. > But there's always been nullblk.ko in kernel that doesn't lie in the > name. If we change the default we are not very honest any more about > what the driver actually does. Then we’re already lying, because if we model it after /dev/null, we should probably return -EIO on every read. If a null device returns data, that data may be arbitrary, so we might as well memset() it to 0. As I wrote in my reply to Daniel, I find it perfectly reasonable to make that the default: For functional tests (which I think are the majority of null’s users), it doesn’t make a difference except that operating on uninitialized data just isn’t a nice thing to do. The only reasons I can see we wouldn’t change the default are (1) compatibility, which I don’t think is an issue for a test driver (plus, the only thing it might break are performance tests, which naively I think wouldn’t be a problem), and (2) it’s an additional gotcha when performance testing, but there are usually so many gotchas with performance testing, that I don’t see this as a problem either. > Even if null-co:// and null-aio:// is a bad idea, then let's add > zero-co://co and zero-aio://, and deprecate null-*://. I find that too much work simply because it’s more work than just making read-zeroes=on the default, and I find doing that reasonable. (Furthermore, we wouldn’t deprecate null-*, because it’s needed for performance testing. We could add read-zeroes as an option to the new zero-* drivers, but I find that silly.) Max ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-23 16:01 ` Max Reitz @ 2021-02-23 17:21 ` Fam Zheng 0 siblings, 0 replies; 24+ messages in thread From: Fam Zheng @ 2021-02-23 17:21 UTC (permalink / raw) To: Max Reitz Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, qemu-block, Markus Armbruster, Wainer dos Santos Moschetta, qemu-devel, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Philippe Mathieu-Daudé On 2021-02-23 17:01, Max Reitz wrote: > On 23.02.21 10:21, Fam Zheng wrote: > > On 2021-02-22 18:55, Philippe Mathieu-Daudé wrote: > > > On 2/22/21 6:35 PM, Fam Zheng wrote: > > > > On 2021-02-19 15:09, Philippe Mathieu-Daudé wrote: > > > > > On 2/19/21 12:07 PM, Max Reitz wrote: > > > > > > On 13.02.21 22:54, Fam Zheng wrote: > > > > > > > On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: > > > > > > > > The null-co driver doesn't zeroize buffer in its default config, > > > > > > > > because it is designed for testing and tests want to run fast. > > > > > > > > However this confuses security researchers (access to uninit > > > > > > > > buffers). > > > > > > > > > > > > > > I'm a little surprised. > > > > > > > > > > > > > > Is changing default the only way to fix this? I'm not opposed to > > > > > > > changing the default but I'm not convinced this is the easiest way. > > > > > > > block/nvme.c also doesn't touch the memory, but defers to the device > > > > > > > DMA, why doesn't that confuse the security checker? > > > > > > > > > > Generally speaking, there is a balance between security and performance. > > > > > We try to provide both, but when we can't, my understanding is security > > > > > is more important. > > > > > > > > Why is hiding the code path behind a non-default more secure? What is > > > > not secure now? > > > > > > Se we are back to the problem of having default values. > > > > > > I'd like to remove the default and have the option explicit, > > > but qemu_opt_get_bool() expects a 'default' value. > > > > > > Should we rename qemu_opt_get_bool() -> qemu_opt_get_bool_with_default() > > > and add a simpler qemu_opt_get_bool()? > > > > My point is I still don't get the full context of the problem this > > series is trying to solve. If the problem is tools are confused, I would > > like to understand why. What is the thing that matters here, exactly? > > My personal opinion is that it isn’t even about tools, it’s just about the > fact that operating on uninitialized data is something that should generally > be avoided. For the null drivers, there is a reason to allow it > (performance testing), but that should be a special case, not the default. How do we define uninitialized data? Are buffers passed to a successful read (2) syscall initialized? We cannot know, because it's up to the fs/driver/storage. It's the same to bdrv_pread(). In fact block/null.c doesn't operate on uninitialized data, we should really fix guess_disk_lchs() and similar. > > > But there's always been nullblk.ko in kernel that doesn't lie in the > > name. If we change the default we are not very honest any more about > > what the driver actually does. > > Then we’re already lying, because if we model it after /dev/null, we should > probably return -EIO on every read. nullblk.ko is not /dev/null, it's /dev/nullb*: https://www.kernel.org/doc/Documentation/block/null_blk.txt Fam ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-19 14:09 ` Philippe Mathieu-Daudé 2021-02-22 17:35 ` Fam Zheng @ 2021-02-22 18:15 ` Daniel P. Berrangé 2021-02-22 18:36 ` Philippe Mathieu-Daudé 2021-02-23 8:44 ` Max Reitz 1 sibling, 2 replies; 24+ messages in thread From: Daniel P. Berrangé @ 2021-02-22 18:15 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, qemu-devel, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Paolo Bonzini, Stefan Hajnoczi, Cleber Rosa, Kevin Wolf On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote: > On 2/19/21 12:07 PM, Max Reitz wrote: > > On 13.02.21 22:54, Fam Zheng wrote: > >> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: > >>> The null-co driver doesn't zeroize buffer in its default config, > >>> because it is designed for testing and tests want to run fast. > >>> However this confuses security researchers (access to uninit > >>> buffers). > >> > >> I'm a little surprised. > >> > >> Is changing default the only way to fix this? I'm not opposed to > >> changing the default but I'm not convinced this is the easiest way. > >> block/nvme.c also doesn't touch the memory, but defers to the device > >> DMA, why doesn't that confuse the security checker? > > Generally speaking, there is a balance between security and performance. > We try to provide both, but when we can't, my understanding is security > is more important. > > Customers expect a secure product. If they prefer performance and > at the price of security, it is also possible by enabling an option > that is not the default. > > I'm not sure why you mention block/nvme here. I have the understanding > the null-co driver is only useful for testing. Are there production > cases where null-co is used? Do we have any real world figures for the performance of null-co with & without zero'ing ? Before worrying about a tradeoff of security vs performance, it'd be good to know if there is actually a real world performance problem in the first place. Personally I'd go for zero'ing by defualt unless the performance hit was really bad. > BTW block/nvme is a particular driver where performance matters more > than security (but we have to make sure the users are aware of that). Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-22 18:15 ` Daniel P. Berrangé @ 2021-02-22 18:36 ` Philippe Mathieu-Daudé 2021-02-23 8:44 ` Max Reitz 1 sibling, 0 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-22 18:36 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, Markus Armbruster, qemu-devel, Wainer dos Santos Moschetta, Max Reitz, Alexander Bulekov, Bandan Das, Paolo Bonzini, Stefan Hajnoczi, Cleber Rosa, Kevin Wolf On 2/22/21 7:15 PM, Daniel P. Berrangé wrote: > On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote: >> On 2/19/21 12:07 PM, Max Reitz wrote: >>> On 13.02.21 22:54, Fam Zheng wrote: >>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: >>>>> The null-co driver doesn't zeroize buffer in its default config, >>>>> because it is designed for testing and tests want to run fast. >>>>> However this confuses security researchers (access to uninit >>>>> buffers). >>>> >>>> I'm a little surprised. >>>> >>>> Is changing default the only way to fix this? I'm not opposed to >>>> changing the default but I'm not convinced this is the easiest way. >>>> block/nvme.c also doesn't touch the memory, but defers to the device >>>> DMA, why doesn't that confuse the security checker? >> >> Generally speaking, there is a balance between security and performance. >> We try to provide both, but when we can't, my understanding is security >> is more important. >> >> Customers expect a secure product. If they prefer performance and >> at the price of security, it is also possible by enabling an option >> that is not the default. >> >> I'm not sure why you mention block/nvme here. I have the understanding >> the null-co driver is only useful for testing. Are there production >> cases where null-co is used? > > Do we have any real world figures for the performance of null-co > with & without zero'ing ? Before worrying about a tradeoff of > security vs performance, it'd be good to know if there is actually > a real world performance problem in the first place. Personally I'd > go for zero'ing by defualt unless the performance hit was really > bad. I simply wanted to help the security team by removing the amount of reports using the null-co driver. This is probably easier to resolve with one documentation line. As I am not understanding where this thread is heading, I am taking a step back with this topic. Please disregard this series. Regards, Phil. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-22 18:15 ` Daniel P. Berrangé 2021-02-22 18:36 ` Philippe Mathieu-Daudé @ 2021-02-23 8:44 ` Max Reitz 2021-02-23 9:29 ` Daniel P. Berrangé 1 sibling, 1 reply; 24+ messages in thread From: Max Reitz @ 2021-02-23 8:44 UTC (permalink / raw) To: Daniel P. Berrangé, Philippe Mathieu-Daudé Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Alexander Bulekov, Bandan Das, Paolo Bonzini, Stefan Hajnoczi, Cleber Rosa, Kevin Wolf On 22.02.21 19:15, Daniel P. Berrangé wrote: > On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote: >> On 2/19/21 12:07 PM, Max Reitz wrote: >>> On 13.02.21 22:54, Fam Zheng wrote: >>>> On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: >>>>> The null-co driver doesn't zeroize buffer in its default config, >>>>> because it is designed for testing and tests want to run fast. >>>>> However this confuses security researchers (access to uninit >>>>> buffers). >>>> >>>> I'm a little surprised. >>>> >>>> Is changing default the only way to fix this? I'm not opposed to >>>> changing the default but I'm not convinced this is the easiest way. >>>> block/nvme.c also doesn't touch the memory, but defers to the device >>>> DMA, why doesn't that confuse the security checker? >> >> Generally speaking, there is a balance between security and performance. >> We try to provide both, but when we can't, my understanding is security >> is more important. >> >> Customers expect a secure product. If they prefer performance and >> at the price of security, it is also possible by enabling an option >> that is not the default. >> >> I'm not sure why you mention block/nvme here. I have the understanding >> the null-co driver is only useful for testing. Are there production >> cases where null-co is used? > > Do we have any real world figures for the performance of null-co > with & without zero'ing ? Before worrying about a tradeoff of > security vs performance, it'd be good to know if there is actually > a real world performance problem in the first place. Personally I'd > go for zero'ing by defualt unless the performance hit was really > bad. AFAIU, null-co is only used for testing, be it to just create some block nodes in the iotests, or perhaps for performance testing where you want to get the minimal roundtrip time through the block layer. So there is no "real world performance problem", because there is no real world use of null-co or null-aio. At least there shouldn’t be. That begs the question of whether read-zeroes=off even makes sense, and I think it absolutely does. In cases where we have a test that just wants a simple block node that doesn’t use disk space, the memset() can’t be noticeable. But it’s just a test, so do we even need the memset()? Strictly speaking, perhaps not, but if someone is to run it via Valgrind or something, they may get false positives, so just doing the memset() is the right thing to do. For performance tests, it must be possible to set read-zeroes=off, because even though “that memset() isn’t noticeable in a functional test”, in a hard-core performance test, it will be. So we need a switch. It should default to memset(), because (1) making tools like Valgrind happy seems like a reasonable objective to me, and (2) in the majority of cases, the memset() cannot have a noticeable impact. Max ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver 2021-02-23 8:44 ` Max Reitz @ 2021-02-23 9:29 ` Daniel P. Berrangé 0 siblings, 0 replies; 24+ messages in thread From: Daniel P. Berrangé @ 2021-02-23 9:29 UTC (permalink / raw) To: Max Reitz Cc: Fam Zheng, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel, Wainer dos Santos Moschetta, Markus Armbruster, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Cleber Rosa, Paolo Bonzini, Kevin Wolf, Philippe Mathieu-Daudé On Tue, Feb 23, 2021 at 09:44:51AM +0100, Max Reitz wrote: > On 22.02.21 19:15, Daniel P. Berrangé wrote: > > On Fri, Feb 19, 2021 at 03:09:43PM +0100, Philippe Mathieu-Daudé wrote: > > > On 2/19/21 12:07 PM, Max Reitz wrote: > > > > On 13.02.21 22:54, Fam Zheng wrote: > > > > > On 2021-02-11 15:26, Philippe Mathieu-Daudé wrote: > > > > > > The null-co driver doesn't zeroize buffer in its default config, > > > > > > because it is designed for testing and tests want to run fast. > > > > > > However this confuses security researchers (access to uninit > > > > > > buffers). > > > > > > > > > > I'm a little surprised. > > > > > > > > > > Is changing default the only way to fix this? I'm not opposed to > > > > > changing the default but I'm not convinced this is the easiest way. > > > > > block/nvme.c also doesn't touch the memory, but defers to the device > > > > > DMA, why doesn't that confuse the security checker? > > > > > > Generally speaking, there is a balance between security and performance. > > > We try to provide both, but when we can't, my understanding is security > > > is more important. > > > > > > Customers expect a secure product. If they prefer performance and > > > at the price of security, it is also possible by enabling an option > > > that is not the default. > > > > > > I'm not sure why you mention block/nvme here. I have the understanding > > > the null-co driver is only useful for testing. Are there production > > > cases where null-co is used? > > > > Do we have any real world figures for the performance of null-co > > with & without zero'ing ? Before worrying about a tradeoff of > > security vs performance, it'd be good to know if there is actually > > a real world performance problem in the first place. Personally I'd > > go for zero'ing by defualt unless the performance hit was really > > bad. > > AFAIU, null-co is only used for testing, be it to just create some block > nodes in the iotests, or perhaps for performance testing where you want to > get the minimal roundtrip time through the block layer. So there is no > "real world performance problem", because there is no real world use of > null-co or null-aio. At least there shouldn’t be. > > That begs the question of whether read-zeroes=off even makes sense, and I > think it absolutely does. > > In cases where we have a test that just wants a simple block node that > doesn’t use disk space, the memset() can’t be noticeable. But it’s just a > test, so do we even need the memset()? Strictly speaking, perhaps not, but > if someone is to run it via Valgrind or something, they may get false > positives, so just doing the memset() is the right thing to do. > > For performance tests, it must be possible to set read-zeroes=off, because > even though “that memset() isn’t noticeable in a functional test”, in a > hard-core performance test, it will be. > > So we need a switch. It should default to memset(), because (1) making > tools like Valgrind happy seems like a reasonable objective to me, and (2) > in the majority of cases, the memset() cannot have a noticeable impact. Yes, that all makes sense to me. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-02-23 17:24 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-11 14:26 [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Philippe Mathieu-Daudé 2021-02-11 14:26 ` [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' Philippe Mathieu-Daudé 2021-02-11 16:29 ` Vladimir Sementsov-Ogievskiy 2021-02-11 19:19 ` Philippe Mathieu-Daudé 2021-02-11 22:40 ` Eric Blake 2021-02-11 23:49 ` Philippe Mathieu-Daudé 2021-02-12 11:34 ` Vladimir Sementsov-Ogievskiy 2021-02-12 19:06 ` Eric Blake 2021-02-11 14:26 ` [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default Philippe Mathieu-Daudé 2021-02-11 16:22 ` Stefan Hajnoczi 2021-02-11 15:42 ` [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver Alexander Bulekov 2021-02-12 14:32 ` Philippe Mathieu-Daudé 2021-02-13 21:54 ` Fam Zheng 2021-02-19 11:07 ` Max Reitz 2021-02-19 14:09 ` Philippe Mathieu-Daudé 2021-02-22 17:35 ` Fam Zheng 2021-02-22 17:55 ` Philippe Mathieu-Daudé 2021-02-23 9:21 ` Fam Zheng 2021-02-23 16:01 ` Max Reitz 2021-02-23 17:21 ` Fam Zheng 2021-02-22 18:15 ` Daniel P. Berrangé 2021-02-22 18:36 ` Philippe Mathieu-Daudé 2021-02-23 8:44 ` Max Reitz 2021-02-23 9:29 ` Daniel P. Berrangé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.