Re: [PATCH] hw/scsi/megasas: Simplify using the ldst API

[Richard Henderson]

On 12/17/21 3:15 PM, Philippe Mathieu-Daudé wrote:

-cdb[3] = (len >> 8) & 0xff;
-cdb[4] = (len & 0xff);
+stw_be_p([2], len);


Wrong offset.  Otherwise,

Reviewed-by: Richard Henderson 

r~

[PATCH] hw/scsi/megasas: Simplify using the ldst API

[Philippe Mathieu-Daudé]

This code is easier to review using the load/store API.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/scsi/megasas.c | 17 +++--
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 619b66ef0f3..066f30e3f22 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -375,8 +375,7 @@ static int megasas_setup_inquiry(uint8_t *cdb, int pg, int 
len)
 cdb[1] = 0x1;
 cdb[2] = pg;
 }
-cdb[3] = (len >> 8) & 0xff;
-cdb[4] = (len & 0xff);
+stw_be_p([2], len);
 return len;
 }
 
@@ -392,18 +391,8 @@ static void megasas_encode_lba(uint8_t *cdb, uint64_t lba,
 } else {
 cdb[0] = READ_16;
 }
-cdb[2] = (lba >> 56) & 0xff;
-cdb[3] = (lba >> 48) & 0xff;
-cdb[4] = (lba >> 40) & 0xff;
-cdb[5] = (lba >> 32) & 0xff;
-cdb[6] = (lba >> 24) & 0xff;
-cdb[7] = (lba >> 16) & 0xff;
-cdb[8] = (lba >> 8) & 0xff;
-cdb[9] = (lba) & 0xff;
-cdb[10] = (len >> 24) & 0xff;
-cdb[11] = (len >> 16) & 0xff;
-cdb[12] = (len >> 8) & 0xff;
-cdb[13] = (len) & 0xff;
+stq_be_p([2], lba);
+stl_be_p([2 + 8], len);
 }
 
 /*
-- 
2.33.1

[PATCH v2 2/2] qemu-img: make is_allocated_sectors() more efficient

[Vladimir Sementsov-Ogievskiy]

Consider the case when the whole buffer is zero and end is unaligned.

If i <= tail, we return 1 and do one unaligned WRITE, RMW happens.

If i > tail, we do on aligned WRITE_ZERO (or skip if target is zeroed)
and again one unaligned WRITE, RMW happens.

Let's do better: don't fragment the whole-zero buffer and report it as
ZERO: in case of zeroed target we just do nothing and avoid RMW. If
target is not zeroes, one unaligned WRITE_ZERO should not be much worse
than one unaligned WRITE.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-img.c | 23 +++
 tests/qemu-iotests/122.out |  8 ++--
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index f036a1d428..d7ddfcc528 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1171,19 +1171,34 @@ static int is_allocated_sectors(const uint8_t *buf, int 
n, int *pnum,
 }
 }
 
+if (i == n) {
+/*
+ * The whole buf is the same.
+ * No reason to split it into chunks, so return now.
+ */
+*pnum = i;
+return !is_zero;
+}
+
 tail = (sector_num + i) & (alignment - 1);
 if (tail) {
 if (is_zero && i <= tail) {
-/* treat unallocated areas which only consist
- * of a small tail as allocated. */
+/*
+ * For sure next sector after i is data, and it will rewrite this
+ * tail anyway due to RMW. So, let's just write data now.
+ */
 is_zero = false;
 }
 if (!is_zero) {
-/* align up end offset of allocated areas. */
+/* If possible, align up end offset of allocated areas. */
 i += alignment - tail;
 i = MIN(i, n);
 } else {
-/* align down end offset of zero areas. */
+/*
+ * For sure next sector after i is data, and it will rewrite this
+ * tail anyway due to RMW. Better is avoid RMW and write zeroes up
+ * to aligned bound.
+ */
 i -= tail;
 }
 }
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 69b8e8b803..e18766e167 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -201,9 +201,7 @@ convert -S 4k
 { "start": 8192, "length": 4096, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
 { "start": 12288, "length": 4096, "depth": 0, "present": false, "zero": true, 
"data": false},
 { "start": 16384, "length": 4096, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
-{ "start": 20480, "length": 46080, "depth": 0, "present": false, "zero": true, 
"data": false},
-{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
-{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": 
true, "data": false}]
+{ "start": 20480, "length": 67088384, "depth": 0, "present": false, "zero": 
true, "data": false}]
 
 convert -c -S 4k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true},
@@ -215,9 +213,7 @@ convert -c -S 4k
 
 convert -S 8k
 [{ "start": 0, "length": 24576, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
-{ "start": 24576, "length": 41984, "depth": 0, "present": false, "zero": true, 
"data": false},
-{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
-{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": 
true, "data": false}]
+{ "start": 24576, "length": 67084288, "depth": 0, "present": false, "zero": 
true, "data": false}]
 
 convert -c -S 8k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true},
-- 
2.31.1

[PATCH v2 0/2] qemu-img convert: Fix sparseness detection

[Vladimir Sementsov-Ogievskiy]

Hi all!

01: only update test output rebasing on master
02: replaced with my proposed solution.

Kevin Wolf (1):
  iotests: Test qemu-img convert of zeroed data cluster

Vladimir Sementsov-Ogievskiy (1):
  qemu-img: make is_allocated_sectors() more efficient

 qemu-img.c | 23 +++
 tests/qemu-iotests/122 |  1 +
 tests/qemu-iotests/122.out |  2 ++
 3 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] iotests: Test qemu-img convert of zeroed data cluster

[Vladimir Sementsov-Ogievskiy]

From: Kevin Wolf 

This demonstrates what happens when the block status changes in
sub-min_sparse granularity, but all of the parts are zeroed out. The
alignment logic in is_allocated_sectors() prevents that the target image
remains fully sparse as expected, but turns it into a data cluster of
explicit zeros.

Signed-off-by: Kevin Wolf 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/122 |  1 +
 tests/qemu-iotests/122.out | 10 --
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index efb260d822..be0f6b79e5 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -251,6 +251,7 @@ $QEMU_IO -c "write -P 0 0 64k" "$TEST_IMG" 2>&1 | 
_filter_qemu_io | _filter_test
 $QEMU_IO -c "write 0 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
 $QEMU_IO -c "write 8k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
 $QEMU_IO -c "write 17k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+$QEMU_IO -c "write -P 0 65k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | 
_filter_testdir
 
 for min_sparse in 4k 8k; do
 echo
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 8fbdac2b39..69b8e8b803 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -192,6 +192,8 @@ wrote 1024/1024 bytes at offset 8192
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1024/1024 bytes at offset 17408
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 66560
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 convert -S 4k
 [{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
@@ -199,7 +201,9 @@ convert -S 4k
 { "start": 8192, "length": 4096, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
 { "start": 12288, "length": 4096, "depth": 0, "present": false, "zero": true, 
"data": false},
 { "start": 16384, "length": 4096, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
-{ "start": 20480, "length": 67088384, "depth": 0, "present": false, "zero": 
true, "data": false}]
+{ "start": 20480, "length": 46080, "depth": 0, "present": false, "zero": true, 
"data": false},
+{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
+{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": 
true, "data": false}]
 
 convert -c -S 4k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true},
@@ -211,7 +215,9 @@ convert -c -S 4k
 
 convert -S 8k
 [{ "start": 0, "length": 24576, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
-{ "start": 24576, "length": 67084288, "depth": 0, "present": false, "zero": 
true, "data": false}]
+{ "start": 24576, "length": 41984, "depth": 0, "present": false, "zero": true, 
"data": false},
+{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true, "offset": OFFSET},
+{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": 
true, "data": false}]
 
 convert -c -S 8k
 [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, 
"data": true},
-- 
2.31.1

Re: [PATCH v5 28/31] block.c: assert BQL lock held in bdrv_co_invalidate_cache

[Emanuele Giuseppe Esposito]

On 17/12/2021 12:04, Hanna Reitz wrote:

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:

bdrv_co_invalidate_cache is special: it is an I/O function,


I still don’t believe it is, but well.

(Yes, it is called by a test in an iothread, but I believe we’ve seen 
that the tests simply sometimes test things that shouldn’t be allowed.)



but uses the block layer permission API, which is GS.

Because of this, we can assert that either the function is
being called with BQL held, and thus can use the permission API,
or make sure that the permission API is not used, by ensuring that
bs (and parents) .open_flags does not contain BDRV_O_INACTIVE.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  block.c | 26 ++
  1 file changed, 26 insertions(+)

diff --git a/block.c b/block.c
index a0309f827d..805974676b 100644
--- a/block.c
+++ b/block.c
@@ -6574,6 +6574,26 @@ void bdrv_init_with_whitelist(void)
  bdrv_init();
  }
+static bool bdrv_is_active(BlockDriverState *bs)
+{
+    BdrvChild *parent;
+
+    if (bs->open_flags & BDRV_O_INACTIVE) {
+    return false;
+    }
+
+    QLIST_FOREACH(parent, >parents, next_parent) {
+    if (parent->klass->parent_is_bds) {
+    BlockDriverState *parent_bs = parent->opaque;


This looks like a really bad hack to me.  We purposefully have made the 
parent link opaque so that a BDS cannot easily reach its parents.  All 
accesses should go through BdrvChildClass methods.


I also don’t understand why we need to query parents at all.  The only 
fact that determines whether the current BDS will have its permissions 
changed is whether the BDS itself is active or inactive.  Sure, we’ll 
invoke bdrv_co_invalidate_cache() on the parents, too, but then we could 
simply let the assertion fail there.



+    if (!bdrv_is_active(parent_bs)) {
+    return false;
+    }
+    }
+    }
+
+   return true;
+}
+
  int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, 
Error **errp)

  {
  BdrvChild *child, *parent;
@@ -6585,6 +6605,12 @@ int coroutine_fn 
bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)

  return -ENOMEDIUM;
  }
+    /*
+ * No need to muck with permissions if bs is active.
+ * TODO: should activation be a separate function?
+ */
+    assert(qemu_in_main_thread() || bdrv_is_active(bs));
+


I don’t understand this, really.  It looks to me like “if you don’t call 
this in the main thread, this better be a no-op”, i.e., you must never 
call this function in an I/O thread if you really want to use it.  I.e. 
what I’d classify as a GS function.


It sounds like this is just a special case for said test, and 
special-casing code for tests sounds like a bad idea.


Ok, but trying to leave just the qemu_in_main_thread() assertion makes 
test 307 (./check 307) fail.
I am actually not sure on why it fails, but I am sure it is because of 
the assertion, since without it it passes.


I tried with gdb (./check -gdb 307 on one terminal and
gdb -iex "target remote localhost:12345"
in another) but it points me to this below, which I think is the ndb 
server getting the socket closed (because on the other side it crashed), 
and not the actual error.



Thread 1 "qemu-system-x86" received signal SIGPIPE, Broken pipe.
0x768af54d in sendmsg () from target:/lib64/libc.so.6
(gdb) bt
#0  0x768af54d in sendmsg () from target:/lib64/libc.so.6
#1  0x55c13cc9 in qio_channel_socket_writev (ioc=out>, iov=0x569a4870, niov=1, fds=0x0, nfds=, errp=0x0)

at ../io/channel-socket.c:561
#2  0x55c19b18 in qio_channel_writev_full_all 
(ioc=0x5763b800, iov=iov@entry=0x7fffe8dffd80, niov=niov@entry=1, 
fds=fds@entry=0x0,

nfds=nfds@entry=0, errp=errp@entry=0x0) at ../io/channel.c:240
#3  0x55c19bd2 in qio_channel_writev_all (errp=0x0, niov=1, 
iov=0x7fffe8dffd80, ioc=) at ../io/channel.c:220
#4  qio_channel_write_all (ioc=, 
buf=buf@entry=0x7fffe8dffdd0 "", buflen=buflen@entry=20, 
errp=errp@entry=0x0) at ../io/channel.c:330
#5  0x55c27e75 in nbd_write (errp=0x0, size=20, 
buffer=0x7fffe8dffdd0, ioc=) at ../nbd/nbd-internal.h:71
#6  nbd_negotiate_send_rep_len (client=client@entry=0x56f60930, 
type=type@entry=1, len=len@entry=0, errp=errp@entry=0x0) at 
../nbd/server.c:203
#7  0x55c29db1 in nbd_negotiate_send_rep (errp=0x0, type=1, 
client=0x56f60930) at ../nbd/server.c:211

--Type  for more, q to quit, c to continue without paging--
#8  nbd_negotiate_options (errp=0x7fffe8dffe88, client=) 
at ../nbd/server.c:1224
#9  nbd_negotiate (errp=0x7fffe8dffe88, client=) at 
../nbd/server.c:1340

#10 nbd_co_client_start (opaque=) at ../nbd/server.c:2715
#11 0x55d70423 in coroutine_trampoline (i0=, 
i1=) at ../util/coroutine-ucontext.c:173

#12 0x767f3820 in ?? () from target:/lib64/libc.so.6
#13 0x7fffca80 in ?? ()

Emanuele


Hanna


  QLIST_FOREACH(child, >children, next) {
  

Re: [PATCH v2 25/25] python: rename qemu.aqmp to qemu.qmp

[John Snow]

On Fri, Dec 17, 2021, 2:40 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 17.12.2021 00:10, John Snow wrote:
> >
> >
> > On Thu, Dec 16, 2021 at 6:41 AM Vladimir Sementsov-Ogievskiy <
> vsement...@virtuozzo.com > wrote:
> >
> > 15.12.2021 22:39, John Snow wrote:
> >  > Now that we are fully switched over to the new QMP library, move
> it back
> >  > over the old namespace. This is being done primarily so that we
> may
> >  > upload this package simply as "qemu.qmp" without introducing
> confusion
> >  > over whether or not "aqmp" is a new protocol or not.
> >  >
> >  > The trade-off is increased confusion inside the QEMU developer
> >  > tree. Sorry!
> >  >
> >  > Signed-off-by: John Snow js...@redhat.com>>
> >
> > Great job!
> >
> > I looked thorough the patch, changes looks correct. Simply rename
> every aqmp / AQMP occurrence.. But:
> >
> >
> > [root@kvm review]# git grep -i aqmp
> > python/qemu/qmp/aqmp_tui.py:AQMP TUI
> > python/qemu/qmp/aqmp_tui.py:AQMP TUI is an asynchronous interface
> built on top the of the AQMP library.
> > python/qemu/qmp/aqmp_tui.py:Example Usage: aqmp-tui  IP:PORT>
> > python/qemu/qmp/aqmp_tui.py:Full Usage: aqmp-tui --help
> > python/qemu/qmp/aqmp_tui.py:Implements the AQMP TUI.
> > python/qemu/qmp/aqmp_tui.py:parser =
> argparse.ArgumentParser(description='AQMP TUI')
> > python/qemu/qmp/legacy.py:self._aqmp = QMPClient(nickname)
> > python/qemu/qmp/legacy.py:if self._aqmp.greeting is not None:
> > python/qemu/qmp/legacy.py:return
> self._aqmp.greeting._asdict()
> > python/qemu/qmp/legacy.py:self._aqmp.await_greeting =
> negotiate
> > python/qemu/qmp/legacy.py:self._aqmp.negotiate = negotiate
> > python/qemu/qmp/legacy.py:
> self._aqmp.connect(self._address)
> > python/qemu/qmp/legacy.py:self._aqmp.await_greeting = True
> > python/qemu/qmp/legacy.py:self._aqmp.negotiate = True
> > python/qemu/qmp/legacy.py:
> self._aqmp.accept(self._address),
> > python/qemu/qmp/legacy.py:self._aqmp._raw(qmp_cmd,
> assign_id=False),
> > python/qemu/qmp/legacy.py:self._aqmp.execute(cmd, kwds),
> > python/qemu/qmp/legacy.py:if self._aqmp.events.empty():
> > python/qemu/qmp/legacy.py:self._aqmp.events.get(),
> > python/qemu/qmp/legacy.py:events = [dict(x) for x in
> self._aqmp.events.clear()]
> > python/qemu/qmp/legacy.py:self._aqmp.events.clear()
> > python/qemu/qmp/legacy.py:self._aqmp.disconnect()
> > python/qemu/qmp/legacy.py:self._aqmp.send_fd_scm(fd)
> > python/qemu/qmp/legacy.py:if self._aqmp.runstate ==
> Runstate.IDLE:
> > python/setup.cfg:# AQMP TUI dependencies
> > python/setup.cfg:aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
> > python/setup.cfg:[mypy-qemu.qmp.aqmp_tui]
> >
> > [root@kvm review]# git ls-tree -r --name-only HEAD | grep -i aqmp
> > python/qemu/qmp/aqmp_tui.py
> >
> >
> > I think, this all should be renamed too
> >
> >
> > For aqmp_tui.py, sure. The new TUI isn't 100% ready to replace qmp-shell
> yet, so I wasn't entirely certain what to name it... qmp-tui?
> >
> > *shrugs*.
>
> I don't remember what tui is abbreviating) qmp-tui is OK, and it may be
> renamed to qmp-shell when it is ready to replace it..
>

"text user interface", by analogy with GUI (graphical UI).


> >
> > for legacy.py, it's just an internal variable name and I wasn't sure it
> was worth the churn just to change a private variable. I could still do it
> if you feel strongly about it.
> >
>
> I'd rename everything.
>

Alright, I'll do so in the respin.


>
> --
> Best regards,
> Vladimir
>

Thanks for the reviews!

>

Re: [PATCH v5 22/31] block_int-common.h: assertion in the callers of BlockDriver function pointers

[Hanna Reitz]

On 17.12.21 16:53, Emanuele Giuseppe Esposito wrote:



On 16/12/2021 19:43, Hanna Reitz wrote:

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  block.c    | 18 ++
  block/create.c | 10 ++
  2 files changed, 28 insertions(+)


[...]


diff --git a/block/create.c b/block/create.c
index 89812669df..0167118579 100644
--- a/block/create.c
+++ b/block/create.c
@@ -42,6 +42,16 @@ static int coroutine_fn blockdev_create_run(Job 
*job, Error **errp)
  BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, 
common);

  int ret;
+    /*
+ * Currently there is nothing preventing this
+ * function from being called in an iothread context.
+ * However, since it will crash anyways because of the
+ * aiocontext lock not taken, we might as well make it
+ * crash with a more meaningful error, by checking that
+ * we are in the main loop
+ */
+    assert(qemu_in_main_thread());


Mostly agreed.  This function is always run in the main loop right 
now, so this assertion will never fail.


But that’s the “mostly”: Adding this assertion won’t give a more 
meaningful error, because the problem still remains that block 
drivers do not error out when encountering (or correctly handle) BDSs 
in non-main contexts, and so it remains a “qemu_mutex_unlock_impl: 
Operation not permitted”.


Not trying to say that that’s your problem.  It’s pre-existing, and 
this assertion is good.  Just wanting to clarify something about the 
comment that seemed unclear to me (in that I found it implied that 
the qemu_mutex_unlock_impl error would be replaced by this assertion 
failing).




You are right. Trying your example given in v4:

$ touch /tmp/iothread-create-test.qcow2
$ ./qemu-system-x86_64 -object iothread,id=iothr0 -qmp stdio <{"execute":"blockdev-add","arguments":{"node-name":"proto","driver":"file","filename":"/tmp/iothread-create-test.qcow2"}} 

{"execute":"x-blockdev-set-iothread","arguments":{"node-name":"proto","iothread":"iothr0"}} 

{"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"proto","size":0}}} 


EOF

I still get "qemu_mutex_unlock_impl: Operation not permitted"

I will remove the comment above the assertion, makes no sense.

Or should I replace it with a TODO/FIXME explaining the above? 
Something like:


/*
 * TODO: it is currently possible to run a blockdev-create job in an
 * I/O thread, for example by doing:
 * [ command line above ]
 * This should not be allowed.
 */


Just removing it makes the most sense to me.  We already have a TODO 
comment to that effect (block/create.c:86).


(And to be precise, it is *not* possible to run a blockdev-create job in 
an I/O thread, it’s always run in the main thread.  It is possible to 
run a blockdev-create job involving nodes in I/O threads, and it’s fine 
that that’s possible, but in such cases the block drivers’ 
.bdrv_co_create() implementations should at least error out with a 
benign error, which they don’t.)


Hanna

Re: [PATCH v5 22/31] block_int-common.h: assertion in the callers of BlockDriver function pointers

[Emanuele Giuseppe Esposito]

On 16/12/2021 19:43, Hanna Reitz wrote:

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  block.c    | 18 ++
  block/create.c | 10 ++
  2 files changed, 28 insertions(+)


[...]


diff --git a/block/create.c b/block/create.c
index 89812669df..0167118579 100644
--- a/block/create.c
+++ b/block/create.c
@@ -42,6 +42,16 @@ static int coroutine_fn blockdev_create_run(Job 
*job, Error **errp)
  BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, 
common);

  int ret;
+    /*
+ * Currently there is nothing preventing this
+ * function from being called in an iothread context.
+ * However, since it will crash anyways because of the
+ * aiocontext lock not taken, we might as well make it
+ * crash with a more meaningful error, by checking that
+ * we are in the main loop
+ */
+    assert(qemu_in_main_thread());


Mostly agreed.  This function is always run in the main loop right now, 
so this assertion will never fail.


But that’s the “mostly”: Adding this assertion won’t give a more 
meaningful error, because the problem still remains that block drivers 
do not error out when encountering (or correctly handle) BDSs in 
non-main contexts, and so it remains a “qemu_mutex_unlock_impl: 
Operation not permitted”.


Not trying to say that that’s your problem.  It’s pre-existing, and this 
assertion is good.  Just wanting to clarify something about the comment 
that seemed unclear to me (in that I found it implied that the 
qemu_mutex_unlock_impl error would be replaced by this assertion failing).




You are right. Trying your example given in v4:

$ touch /tmp/iothread-create-test.qcow2
$ ./qemu-system-x86_64 -object iothread,id=iothr0 -qmp stdio common, 1);
  ret = s->drv->bdrv_co_create(s->opts, errp);
  job_progress_update(>common, 1);

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Daniel P . Berrangé]

On Fri, Dec 17, 2021 at 02:53:05PM +, Alex Bennée wrote:
> 
> Daniel P. Berrangé  writes:
> 
> > On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
> >> 
> >> Philippe Mathieu-Daudé  writes:
> >> 
> >> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> >> > (Fedora 34 provides GLib 2.68.1) we get:
> >> >
> >> >   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
> >> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >> >   ...
> >> >
> >> > g_memdup() has been updated by g_memdup2() to fix eventual security
> >> > issues (size argument is 32-bit and could be truncated / wrapping).
> >> > GLib recommends to copy their static inline version of g_memdup2():
> >> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >> >
> >> > Our glib-compat.h provides a comment explaining how to deal with
> >> > these deprecated declarations (see commit e71e8cc0355
> >> > "glib: enforce the minimum required version and warn about old APIs").
> >> >
> >> > Following this comment suggestion, implement the g_memdup2_qemu()
> >> > wrapper to g_memdup2(), and use the safer equivalent inlined when
> >> > we are using pre-2.68 GLib.
> >> >
> >> > Reported-by: Eric Blake 
> >> > Signed-off-by: Philippe Mathieu-Daudé 
> >> > ---
> >> >  include/glib-compat.h | 37 +
> >> >  1 file changed, 37 insertions(+)
> >> >
> >> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> >> > index 9e95c888f54..8d01a8c01fb 100644
> >> > --- a/include/glib-compat.h
> >> > +++ b/include/glib-compat.h
> >> > @@ -68,6 +68,43 @@
> >> >   * without generating warnings.
> >> >   */
> >> >  
> >> > +/*
> >> > + * g_memdup2_qemu:
> >> > + * @mem: (nullable): the memory to copy.
> >> > + * @byte_size: the number of bytes to copy.
> >> > + *
> >> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes 
> >> > into it
> >> > + * from @mem. If @mem is %NULL it returns %NULL.
> >> > + *
> >> > + * This replaces g_memdup(), which was prone to integer overflows when
> >> > + * converting the argument from a #gsize to a #guint.
> >> > + *
> >> > + * This static inline version is a backport of the new public API from
> >> > + * GLib 2.68, kept internal to GLib for backport to older stable 
> >> > releases.
> >> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> >> > + *
> >> > + * Returns: (nullable): a pointer to the newly-allocated copy of the 
> >> > memory,
> >> > + *  or %NULL if @mem is %NULL.
> >> > + */
> >> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize 
> >> > byte_size)
> >> > +{
> >> > +#if GLIB_CHECK_VERSION(2, 68, 0)
> >> > +return g_memdup2(mem, byte_size);
> >> > +#else
> >> > +gpointer new_mem;
> >> > +
> >> > +if (mem && byte_size != 0) {
> >> > +new_mem = g_malloc(byte_size);
> >> > +memcpy(new_mem, mem, byte_size);
> >> > +} else {
> >> > +new_mem = NULL;
> >> > +}
> >> > +
> >> > +return new_mem;
> >> > +#endif
> >> > +}
> >> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> >> > +
> >> 
> >> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> >> s)?
> >
> > Not in this case. We use suffix as we don't want people calling this
> > directly with the suffix.
> >
> > In the glibcompat.h header we're attempting to transparently/secretly
> > replace/wrap standard glib APIs.  All the callers should remain using
> > the plain glib API name, never call the method with the suffix at
> > all. This lets us delete the wrapper later and not have to update
> > any callers. The suffix is basically just a hack of the impl we use
> > for transparent replacement.
> 
> Right - at the risk of bike shedding names maybe we should choose a
> suffix the better reflects the purpose like _alt or _internal rather
> than overloading qemu?

Sure, I'm fine with 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 :|

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Alex Bennée]

Daniel P. Berrangé  writes:

> On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
>> 
>> Philippe Mathieu-Daudé  writes:
>> 
>> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
>> > (Fedora 34 provides GLib 2.68.1) we get:
>> >
>> >   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
>> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
>> >   ...
>> >
>> > g_memdup() has been updated by g_memdup2() to fix eventual security
>> > issues (size argument is 32-bit and could be truncated / wrapping).
>> > GLib recommends to copy their static inline version of g_memdup2():
>> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>> >
>> > Our glib-compat.h provides a comment explaining how to deal with
>> > these deprecated declarations (see commit e71e8cc0355
>> > "glib: enforce the minimum required version and warn about old APIs").
>> >
>> > Following this comment suggestion, implement the g_memdup2_qemu()
>> > wrapper to g_memdup2(), and use the safer equivalent inlined when
>> > we are using pre-2.68 GLib.
>> >
>> > Reported-by: Eric Blake 
>> > Signed-off-by: Philippe Mathieu-Daudé 
>> > ---
>> >  include/glib-compat.h | 37 +
>> >  1 file changed, 37 insertions(+)
>> >
>> > diff --git a/include/glib-compat.h b/include/glib-compat.h
>> > index 9e95c888f54..8d01a8c01fb 100644
>> > --- a/include/glib-compat.h
>> > +++ b/include/glib-compat.h
>> > @@ -68,6 +68,43 @@
>> >   * without generating warnings.
>> >   */
>> >  
>> > +/*
>> > + * g_memdup2_qemu:
>> > + * @mem: (nullable): the memory to copy.
>> > + * @byte_size: the number of bytes to copy.
>> > + *
>> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into 
>> > it
>> > + * from @mem. If @mem is %NULL it returns %NULL.
>> > + *
>> > + * This replaces g_memdup(), which was prone to integer overflows when
>> > + * converting the argument from a #gsize to a #guint.
>> > + *
>> > + * This static inline version is a backport of the new public API from
>> > + * GLib 2.68, kept internal to GLib for backport to older stable releases.
>> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
>> > + *
>> > + * Returns: (nullable): a pointer to the newly-allocated copy of the 
>> > memory,
>> > + *  or %NULL if @mem is %NULL.
>> > + */
>> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
>> > +{
>> > +#if GLIB_CHECK_VERSION(2, 68, 0)
>> > +return g_memdup2(mem, byte_size);
>> > +#else
>> > +gpointer new_mem;
>> > +
>> > +if (mem && byte_size != 0) {
>> > +new_mem = g_malloc(byte_size);
>> > +memcpy(new_mem, mem, byte_size);
>> > +} else {
>> > +new_mem = NULL;
>> > +}
>> > +
>> > +return new_mem;
>> > +#endif
>> > +}
>> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
>> > +
>> 
>> As per our style wouldn't it make sense to just call it qemu_memdup(m,
>> s)?
>
> Not in this case. We use suffix as we don't want people calling this
> directly with the suffix.
>
> In the glibcompat.h header we're attempting to transparently/secretly
> replace/wrap standard glib APIs.  All the callers should remain using
> the plain glib API name, never call the method with the suffix at
> all. This lets us delete the wrapper later and not have to update
> any callers. The suffix is basically just a hack of the impl we use
> for transparent replacement.

Right - at the risk of bike shedding names maybe we should choose a
suffix the better reflects the purpose like _alt or _internal rather
than overloading qemu?

We already document _locked for example.

> A method with a 'qemu_' prefix by constrast is something that callers
> are explicitly expected to call directly.
>
>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [RFC PATCH 2/2] qemu-img convert: Fix sparseness detection

[Peter Lieven]

Am 04.12.21 um 00:04 schrieb Vladimir Sementsov-Ogievskiy:
> 03.12.2021 14:17, Peter Lieven wrote:
>> Am 19.05.21 um 18:48 schrieb Kevin Wolf:
>>> Am 19.05.2021 um 15:24 hat Peter Lieven geschrieben:
 Am 20.04.21 um 18:52 schrieb Vladimir Sementsov-Ogievskiy:
> 20.04.2021 18:04, Kevin Wolf wrote:
>> Am 20.04.2021 um 16:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 15.04.2021 18:22, Kevin Wolf wrote:
 In order to avoid RMW cycles, is_allocated_sectors() treats zeroed 
 areas
 like non-zero data if the end of the checked area isn't aligned. This
 can improve the efficiency of the conversion and was introduced in
 commit 8dcd3c9b91a.

 However, it comes with a correctness problem: qemu-img convert is
 supposed to sparsify areas that contain only zeros, which it doesn't do
 any more. It turns out that this even happens when not only the
 unaligned area is zeroed, but also the blocks before and after it. In
 the bug report, conversion of a fragmented 10G image containing only
 zeros resulted in an image consuming 2.82 GiB even though the expected
 size is only 4 KiB.

 As a tradeoff between both, let's ignore zeroed sectors only after
 non-zero data to fix the alignment, but if we're only looking at zeros,
 keep them as such, even if it may mean additional RMW cycles.

>>> Hmm.. If I understand correctly, we are going to do unaligned
>>> write-zero. And that helps.
>> This can happen (mostly raw images on block devices, I think?), but
>> usually it just means skipping the write because we know that the target
>> image is already zeroed.
>>
>> What it does mean is that if the next part is data, we'll have an
>> unaligned data write.
>>
>>> Doesn't that mean that alignment is wrongly detected?
>> The problem is that you can have bdrv_block_status_above() return the
>> same allocation status multiple times in a row, but *pnum can be
>> unaligned for the conversion.
>>
>> We only look at a single range returned by it when detecting the
>> alignment, so it could be that we have zero buffers for both 0-11 and
>> 12-16 and detect two misaligned ranges, when both together are a
>> perfectly aligned zeroed range.
>>
>> In theory we could try to do some lookahead and merge ranges where
>> possible, which should give us the perfect result, but it would make the
>> code considerably more complicated. (Whether we want to merge them
>> doesn't only depend on the block status, but possibly also on the
>> content of a DATA range.)
>>
>> Kevin
>>
> Oh, I understand now the problem, thanks for explanation.
>
> Hmm, yes that means, that if the whole buf is zero, is_allocated_sectors 
> must not align it down, to be possibly "merged" with next chunk if it is 
> zero too.
>
> But it's still good to align zeroes down, if data starts somewhere inside 
> the buf, isn't it?
>
> what about something like this:
>
> diff --git a/qemu-img.c b/qemu-img.c
> index babb5573ab..d1704584a0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1167,19 +1167,39 @@ static int is_allocated_sectors(const uint8_t 
> *buf, int n, int *pnum,
>   }
>   }
>   +    if (i == n) {
> +    /*
> + * The whole buf is the same.
> + *
> + * if it's data, just return it. It's the old behavior.
> + *
> + * if it's zero, just return too. It will work good if target is 
> alredy
> + * zeroed. And if next chunk is zero too we'll have no RMW and 
> no reason
> + * to write data.
> + */
> +    *pnum = i;
> +    return !is_zero;
> +    }
> +
>   tail = (sector_num + i) & (alignment - 1);
>   if (tail) {
>   if (is_zero && i <= tail) {
> -    /* treat unallocated areas which only consist
> - * of a small tail as allocated. */
> +    /*
> + * For sure next sector after i is data, and it will rewrite 
> this
> + * tail anyway due to RMW. So, let's just write data now.
> + */
>   is_zero = false;
>   }
>   if (!is_zero) {
> -    /* align up end offset of allocated areas. */
> +    /* If possible, align up end offset of allocated areas. */
>   i += alignment - tail;
>   i = MIN(i, n);
>   } else {
> -    /* align down end offset of zero areas. */
> +    /*
> + * For sure next sector after i is data, and it will rewrite 
> this
> + * tail anyway due to RMW. Better is avoid RMW and write 
> zeroes up

Re: [PATCH v5 30/31] crypto: delegate permission functions to JobDriver .pre_run

[Hanna Reitz]

On 17.12.21 13:29, Hanna Reitz wrote:

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:

block_crypto_amend_options_generic_luks uses the block layer
permission API, therefore it should be called with the BQL held.

However, the same function is being called ib two BlockDriver


s/ ib / by /


callbacks: bdrv_amend_options (under BQL) and bdrv_co_amend (I/O).

The latter is I/O because it is invoked by block/amend.c's
blockdev_amend_run(), a .run callback of the amend JobDriver

Therefore we want to 1) change block_crypto_amend_options_generic_luks
to use the permission API only when the BQL is held, and
2) use the .pre_run JobDriver callback to check for
permissions before switching to the job aiocontext. This has also
the benefit of applying the same permission operation to all
amend implementations, not only luks.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/amend.c  | 20 
  block/crypto.c | 18 --
  2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/block/amend.c b/block/amend.c
index 392df9ef83..fba6add51a 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -53,10 +53,30 @@ static int coroutine_fn blockdev_amend_run(Job 
*job, Error **errp)

  return ret;
  }
  +static int blockdev_amend_refresh_perms(Job *job, Error **errp)
+{
+    BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
+
+    return bdrv_child_refresh_perms(s->bs, s->bs->file, errp);
+}


I miss some documentation for this function, why we do it and how it 
works together with the bdrv_co_amend implementation.


I was trying to come up with an example text, but then I wondered – 
how does it actually work?  bdrv_child_refresh_perms() eventually ends 
up in block_crypto_child_perms().  However, that will only return 
exceptional permissions if crypto->updating_keys is true. But that’s 
set only in block_crypto_amend_options_generic_luks() – i.e. when the 
job runs.  That’s exactly why that function calls 
bdrv_child_refresh_perms() only after it has modified 
crypto->updating_keys.


Reproducer (amend on a LUKS image with read-only=true, so it doesn’t 
have the WRITE permission continuously, but needs to take it as an 
exception in block_crypto_child_perms()):


$ qemu-img create \
    -f luks \
    --object secret,id=sec0,data=123456 \
    -o key-secret=sec0 \
    test.luks \
    64M
Formatting 'test.luks', fmt=luks size=67108864 key-secret=sec0

$ ./qemu-system-x86_64 \
    -object secret,id=sec0,data=123456 \
    -object iothread,id=iothr0 \
    -blockdev file,node-name=node0,filename=test.luks \
    -blockdev 
luks,node-name=node1,key-secret=sec0,file=node0,read-only=true \

    -device virtio-blk,drive=node1,iothread=iothr0 -qmp stdio \
    <{"QMP": {"version": {"qemu": {"micro": 93, "minor": 1, "major": 6}, 
"package": "v6.2.0-rc3-50-gdb635fc4e7"}, "capabilities": ["oob"]}}

{"return": {}}
{"timestamp": {"seconds": 1639742600, "microseconds": 574641}, 
"event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": 
"amend0"}}
{"timestamp": {"seconds": 1639742600, "microseconds": 574919}, 
"event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": 
"amend0"}}

{"return": {}}
qemu-system-x86_64: ../block/io.c:2041: bdrv_co_write_req_prepare: 
Assertion `child->perm & BLK_PERM_WRITE' failed.
[1]    55880 IOT instruction (core dumped)  ./qemu-system-x86_64 
-object secret,id=sec0,data=123456 -object  -blockdev


Addendum: Running the iotests, I realized that (because 295 and 296 fail 
for luks) of course it doesn’t matter whether the job runs in the main 
loop or not in order to reproduce this assertion failure, so the 
`-object iothread,id=iothr0` and `-device 
virtio-blk,drive=node1,iothread=iothr0` can be dropped from the qemu 
command line.


Hanna

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Daniel P . Berrangé]

On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé  writes:
> 
> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> > (Fedora 34 provides GLib 2.68.1) we get:
> >
> >   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >   ...
> >
> > g_memdup() has been updated by g_memdup2() to fix eventual security
> > issues (size argument is 32-bit and could be truncated / wrapping).
> > GLib recommends to copy their static inline version of g_memdup2():
> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >
> > Our glib-compat.h provides a comment explaining how to deal with
> > these deprecated declarations (see commit e71e8cc0355
> > "glib: enforce the minimum required version and warn about old APIs").
> >
> > Following this comment suggestion, implement the g_memdup2_qemu()
> > wrapper to g_memdup2(), and use the safer equivalent inlined when
> > we are using pre-2.68 GLib.
> >
> > Reported-by: Eric Blake 
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  include/glib-compat.h | 37 +
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> > index 9e95c888f54..8d01a8c01fb 100644
> > --- a/include/glib-compat.h
> > +++ b/include/glib-compat.h
> > @@ -68,6 +68,43 @@
> >   * without generating warnings.
> >   */
> >  
> > +/*
> > + * g_memdup2_qemu:
> > + * @mem: (nullable): the memory to copy.
> > + * @byte_size: the number of bytes to copy.
> > + *
> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into 
> > it
> > + * from @mem. If @mem is %NULL it returns %NULL.
> > + *
> > + * This replaces g_memdup(), which was prone to integer overflows when
> > + * converting the argument from a #gsize to a #guint.
> > + *
> > + * This static inline version is a backport of the new public API from
> > + * GLib 2.68, kept internal to GLib for backport to older stable releases.
> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> > + *
> > + * Returns: (nullable): a pointer to the newly-allocated copy of the 
> > memory,
> > + *  or %NULL if @mem is %NULL.
> > + */
> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
> > +{
> > +#if GLIB_CHECK_VERSION(2, 68, 0)
> > +return g_memdup2(mem, byte_size);
> > +#else
> > +gpointer new_mem;
> > +
> > +if (mem && byte_size != 0) {
> > +new_mem = g_malloc(byte_size);
> > +memcpy(new_mem, mem, byte_size);
> > +} else {
> > +new_mem = NULL;
> > +}
> > +
> > +return new_mem;
> > +#endif
> > +}
> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> > +
> 
> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> s)?

Not in this case. We use suffix as we don't want people calling this
directly with the suffix.

In the glibcompat.h header we're attempting to transparently/secretly
replace/wrap standard glib APIs.  All the callers should remain using
the plain glib API name, never call the method with the suffix at
all. This lets us delete the wrapper later and not have to update
any callers. The suffix is basically just a hack of the impl we use
for transparent replacement. 

A method with a 'qemu_' prefix by constrast is something that callers
are explicitly expected to call directly.


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 :|

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Daniel P . Berrangé]

On Fri, Dec 17, 2021 at 11:10:31AM +, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé  writes:
> 
> > On 12/16/21 15:11, Alex Bennée wrote:
> >> Philippe Mathieu-Daudé  writes:
> >> 
> >>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> >>> (Fedora 34 provides GLib 2.68.1) we get:
> >>>
> >>>   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
> >>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >>>   ...
> >>>
> >>> g_memdup() has been updated by g_memdup2() to fix eventual security
> >>> issues (size argument is 32-bit and could be truncated / wrapping).
> >>> GLib recommends to copy their static inline version of g_memdup2():
> >>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >>>
> >>> Our glib-compat.h provides a comment explaining how to deal with
> >>> these deprecated declarations (see commit e71e8cc0355
> >>> "glib: enforce the minimum required version and warn about old APIs").
> >>>
> 
> >>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> >>> +
> >> 
> >> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> >> s)?
> >
> > I followed the documentation in include/glib-compat.h:
> >
> > /*
> >  * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
> > allowing
> >  * use of functions from newer GLib via this compat header needs a little
> >  * trickery to prevent warnings being emitted.
> >  *
> >  * Consider a function from newer glib-X.Y that we want to use
> >  *
> >  *int g_foo(const char *wibble)
> >  *
> >  * We must define a static inline function with the same signature that does
> >  * what we need, but with a "_qemu" suffix e.g.
> >  *
> >  * static inline void g_foo_qemu(const char *wibble)
> >  * {
> >  * #if GLIB_CHECK_VERSION(X, Y, 0)
> >  *g_foo(wibble)
> >  * #else
> >  *g_something_equivalent_in_older_glib(wibble);
> >  * #endif
> >  * }
> >  *
> >  * The #pragma at the top of this file turns off -Wdeprecated-declarations,
> >  * ensuring this wrapper function impl doesn't trigger the compiler warning
> >  * about using too new glib APIs. Finally we can do
> >  *
> >  *   #define g_foo(a) g_foo_qemu(a)
> >  *
> >  * So now the code elsewhere in QEMU, which *does* have the
> >  * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
> >  * without generating warnings.
> >  */
> >
> > which is how g_unix_get_passwd_entry_qemu() is implemented.
> 
> Yet later we have qemu_g_test_slow following the style guide. Also I'm
> confused by the usage of g_unix_get_passwd_entry_qemu because the only
> place I see it in qga/commands-posix-ssh.c right before it does:
> 
> #define g_unix_get_passwd_entry_qemu(username, err) \
>test_get_passwd_entry(username, err)

The g_unix_get_passwd_entry method is a bad example as it is
not following the guide for transparent replacement.

 > 
> although I think that only hold when the file is built with
> QGA_BUILD_UNIT_TEST.
> 
> > Should we reword the documentation first?
> 
> The original wording in glib-compat.h was added by Daniel in 2018 but
> the commit that added the password function comments:
> 
>   Since the fallback version is still unsafe, I would rather keep the
>   _qemu postfix, to make sure it's not being misused by mistake. When/if
>   necessary, we can implement a safer fallback and drop the _qemu suffix.
> 
> So if we are going to make a distinction between a qemu prefix and
> suffix we should agree that and add it to the style document.

The glibcompat.h header should only be used for cases where
we are doing transparent replacement such that callers continue
using the normal glib API name, and the suffix is a hidden
impl detail only seen in the header.

I think in that particular case we should have just had
'qemu_unix_get_passwd_entry' defined in a completely separate
place like osdep.c.

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 :|

Re: [PATCH v5 30/31] crypto: delegate permission functions to JobDriver .pre_run

[Hanna Reitz]

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:

block_crypto_amend_options_generic_luks uses the block layer
permission API, therefore it should be called with the BQL held.

However, the same function is being called ib two BlockDriver


s/ ib / by /


callbacks: bdrv_amend_options (under BQL) and bdrv_co_amend (I/O).

The latter is I/O because it is invoked by block/amend.c's
blockdev_amend_run(), a .run callback of the amend JobDriver

Therefore we want to 1) change block_crypto_amend_options_generic_luks
to use the permission API only when the BQL is held, and
2) use the .pre_run JobDriver callback to check for
permissions before switching to the job aiocontext. This has also
the benefit of applying the same permission operation to all
amend implementations, not only luks.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/amend.c  | 20 
  block/crypto.c | 18 --
  2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/block/amend.c b/block/amend.c
index 392df9ef83..fba6add51a 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -53,10 +53,30 @@ static int coroutine_fn blockdev_amend_run(Job *job, Error 
**errp)
  return ret;
  }
  
+static int blockdev_amend_refresh_perms(Job *job, Error **errp)

+{
+BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
+
+return bdrv_child_refresh_perms(s->bs, s->bs->file, errp);
+}


I miss some documentation for this function, why we do it and how it 
works together with the bdrv_co_amend implementation.


I was trying to come up with an example text, but then I wondered – how 
does it actually work?  bdrv_child_refresh_perms() eventually ends up in 
block_crypto_child_perms().  However, that will only return exceptional 
permissions if crypto->updating_keys is true. But that’s set only in 
block_crypto_amend_options_generic_luks() – i.e. when the job runs.  
That’s exactly why that function calls bdrv_child_refresh_perms() only 
after it has modified crypto->updating_keys.


Reproducer (amend on a LUKS image with read-only=true, so it doesn’t 
have the WRITE permission continuously, but needs to take it as an 
exception in block_crypto_child_perms()):


$ qemu-img create \
    -f luks \
    --object secret,id=sec0,data=123456 \
    -o key-secret=sec0 \
    test.luks \
    64M
Formatting 'test.luks', fmt=luks size=67108864 key-secret=sec0

$ ./qemu-system-x86_64 \
    -object secret,id=sec0,data=123456 \
    -object iothread,id=iothr0 \
    -blockdev file,node-name=node0,filename=test.luks \
    -blockdev 
luks,node-name=node1,key-secret=sec0,file=node0,read-only=true \

    -device virtio-blk,drive=node1,iothread=iothr0 -qmp stdio \
    <{"QMP": {"version": {"qemu": {"micro": 93, "minor": 1, "major": 6}, 
"package": "v6.2.0-rc3-50-gdb635fc4e7"}, "capabilities": ["oob"]}}

{"return": {}}
{"timestamp": {"seconds": 1639742600, "microseconds": 574641}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "amend0"}}
{"timestamp": {"seconds": 1639742600, "microseconds": 574919}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "running", "id": "amend0"}}

{"return": {}}
qemu-system-x86_64: ../block/io.c:2041: bdrv_co_write_req_prepare: 
Assertion `child->perm & BLK_PERM_WRITE' failed.
[1]    55880 IOT instruction (core dumped)  ./qemu-system-x86_64 -object 
secret,id=sec0,data=123456 -object  -blockdev



I believe this means we need some new block driver function to prepare 
for an amendment operation.  If so, another question comes up, which is 
whether this preparatory function should then also call 
bdrv_child_refresh_perms(), and then whether we should have a clean-up 
function for symmetry.



+
+static int blockdev_amend_pre_run(Job *job, Error **errp)
+{
+return blockdev_amend_refresh_perms(job, errp);
+}
+
+static void blockdev_amend_clean(Job *job)
+{
+Error *errp;
+blockdev_amend_refresh_perms(job, );


Do we really want to ignore this error?  If so, we shouldn’t pass a 
pointer to an unused local variable, but NULL.


If we don’t want to ignore it, we have the option of doing what you do 
here and then at least reporting a potential error with 
error_report_err(), and then freeing it, and we also must initialize 
errp to NULL in this case.


If we expect no error to happen (e.g. because we require the amend 
implementation to only release/share permissions and not acquire/unshare 
them), then I’d expect passing _abort here.



+}
+
  static const JobDriver blockdev_amend_job_driver = {
  .instance_size = sizeof(BlockdevAmendJob),
  .job_type  = JOB_TYPE_AMEND,
  .run   = blockdev_amend_run,
+.pre_run   = blockdev_amend_pre_run,
+.clean = blockdev_amend_clean,
  };
  
  void qmp_x_blockdev_amend(const char *job_id,

diff --git a/block/crypto.c b/block/crypto.c
index c8ba4681e2..82f154516c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -780,6 +780,7 @@ block_crypto_get_specific_info_luks(BlockDriverState 

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Alex Bennée]

Laurent Vivier  writes:

> Alex,
>
> I've added this patch to my trivial patches branch, do you want I drop
> it?

No - the patch itself is fine. I would like to fix up the consistency
later if we can agree on what it should be.

-- 
Alex Bennée

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Laurent Vivier]

Alex,

I've added this patch to my trivial patches branch, do you want I drop it?

Thanks,
Laurent

On 17/12/2021 12:10, Alex Bennée wrote:

Philippe Mathieu-Daudé  writes:


On 12/16/21 15:11, Alex Bennée wrote:

Philippe Mathieu-Daudé  writes:


When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:

   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
   ...

g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538

Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").




+#define g_memdup2(m, s) g_memdup2_qemu(m, s)
+

As per our style wouldn't it make sense to just call it qemu_memdup(m,
s)?

I followed the documentation in include/glib-compat.h:

/*
  * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
allowing
  * use of functions from newer GLib via this compat header needs a little
  * trickery to prevent warnings being emitted.
  *
  * Consider a function from newer glib-X.Y that we want to use
  *
  *int g_foo(const char *wibble)
  *
  * We must define a static inline function with the same signature that does
  * what we need, but with a "_qemu" suffix e.g.
  *
  * static inline void g_foo_qemu(const char *wibble)
  * {
  * #if GLIB_CHECK_VERSION(X, Y, 0)
  *g_foo(wibble)
  * #else
  *g_something_equivalent_in_older_glib(wibble);
  * #endif
  * }
  *
  * The #pragma at the top of this file turns off -Wdeprecated-declarations,
  * ensuring this wrapper function impl doesn't trigger the compiler warning
  * about using too new glib APIs. Finally we can do
  *
  *   #define g_foo(a) g_foo_qemu(a)
  *
  * So now the code elsewhere in QEMU, which *does* have the
  * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
  * without generating warnings.
  */

which is how g_unix_get_passwd_entry_qemu() is implemented.

Yet later we have qemu_g_test_slow following the style guide. Also I'm
confused by the usage of g_unix_get_passwd_entry_qemu because the only
place I see it in qga/commands-posix-ssh.c right before it does:

#define g_unix_get_passwd_entry_qemu(username, err) \
test_get_passwd_entry(username, err)

although I think that only hold when the file is built with
QGA_BUILD_UNIT_TEST.


Should we reword the documentation first?

The original wording in glib-compat.h was added by Daniel in 2018 but
the commit that added the password function comments:

   Since the fallback version is still unsafe, I would rather keep the
   _qemu postfix, to make sure it's not being misused by mistake. When/if
   necessary, we can implement a safer fallback and drop the _qemu suffix.

So if we are going to make a distinction between a qemu prefix and
suffix we should agree that and add it to the style document.

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> On 12/16/21 15:11, Alex Bennée wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
>>> (Fedora 34 provides GLib 2.68.1) we get:
>>>
>>>   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
>>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
>>>   ...
>>>
>>> g_memdup() has been updated by g_memdup2() to fix eventual security
>>> issues (size argument is 32-bit and could be truncated / wrapping).
>>> GLib recommends to copy their static inline version of g_memdup2():
>>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>>>
>>> Our glib-compat.h provides a comment explaining how to deal with
>>> these deprecated declarations (see commit e71e8cc0355
>>> "glib: enforce the minimum required version and warn about old APIs").
>>>

>>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
>>> +
>> 
>> As per our style wouldn't it make sense to just call it qemu_memdup(m,
>> s)?
>
> I followed the documentation in include/glib-compat.h:
>
> /*
>  * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
> allowing
>  * use of functions from newer GLib via this compat header needs a little
>  * trickery to prevent warnings being emitted.
>  *
>  * Consider a function from newer glib-X.Y that we want to use
>  *
>  *int g_foo(const char *wibble)
>  *
>  * We must define a static inline function with the same signature that does
>  * what we need, but with a "_qemu" suffix e.g.
>  *
>  * static inline void g_foo_qemu(const char *wibble)
>  * {
>  * #if GLIB_CHECK_VERSION(X, Y, 0)
>  *g_foo(wibble)
>  * #else
>  *g_something_equivalent_in_older_glib(wibble);
>  * #endif
>  * }
>  *
>  * The #pragma at the top of this file turns off -Wdeprecated-declarations,
>  * ensuring this wrapper function impl doesn't trigger the compiler warning
>  * about using too new glib APIs. Finally we can do
>  *
>  *   #define g_foo(a) g_foo_qemu(a)
>  *
>  * So now the code elsewhere in QEMU, which *does* have the
>  * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
>  * without generating warnings.
>  */
>
> which is how g_unix_get_passwd_entry_qemu() is implemented.

Yet later we have qemu_g_test_slow following the style guide. Also I'm
confused by the usage of g_unix_get_passwd_entry_qemu because the only
place I see it in qga/commands-posix-ssh.c right before it does:

#define g_unix_get_passwd_entry_qemu(username, err) \
   test_get_passwd_entry(username, err)

although I think that only hold when the file is built with
QGA_BUILD_UNIT_TEST.

> Should we reword the documentation first?

The original wording in glib-compat.h was added by Daniel in 2018 but
the commit that added the password function comments:

  Since the fallback version is still unsafe, I would rather keep the
  _qemu postfix, to make sure it's not being misused by mistake. When/if
  necessary, we can implement a safer fallback and drop the _qemu suffix.

So if we are going to make a distinction between a qemu prefix and
suffix we should agree that and add it to the style document.

-- 
Alex Bennée

Re: [PATCH v5 28/31] block.c: assert BQL lock held in bdrv_co_invalidate_cache

[Hanna Reitz]

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:

bdrv_co_invalidate_cache is special: it is an I/O function,


I still don’t believe it is, but well.

(Yes, it is called by a test in an iothread, but I believe we’ve seen 
that the tests simply sometimes test things that shouldn’t be allowed.)



but uses the block layer permission API, which is GS.

Because of this, we can assert that either the function is
being called with BQL held, and thus can use the permission API,
or make sure that the permission API is not used, by ensuring that
bs (and parents) .open_flags does not contain BDRV_O_INACTIVE.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  block.c | 26 ++
  1 file changed, 26 insertions(+)

diff --git a/block.c b/block.c
index a0309f827d..805974676b 100644
--- a/block.c
+++ b/block.c
@@ -6574,6 +6574,26 @@ void bdrv_init_with_whitelist(void)
  bdrv_init();
  }
  
+static bool bdrv_is_active(BlockDriverState *bs)

+{
+BdrvChild *parent;
+
+if (bs->open_flags & BDRV_O_INACTIVE) {
+return false;
+}
+
+QLIST_FOREACH(parent, >parents, next_parent) {
+if (parent->klass->parent_is_bds) {
+BlockDriverState *parent_bs = parent->opaque;


This looks like a really bad hack to me.  We purposefully have made the 
parent link opaque so that a BDS cannot easily reach its parents.  All 
accesses should go through BdrvChildClass methods.


I also don’t understand why we need to query parents at all.  The only 
fact that determines whether the current BDS will have its permissions 
changed is whether the BDS itself is active or inactive.  Sure, we’ll 
invoke bdrv_co_invalidate_cache() on the parents, too, but then we could 
simply let the assertion fail there.



+if (!bdrv_is_active(parent_bs)) {
+return false;
+}
+}
+}
+
+   return true;
+}
+
  int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
  {
  BdrvChild *child, *parent;
@@ -6585,6 +6605,12 @@ int coroutine_fn 
bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
  return -ENOMEDIUM;
  }
  
+/*

+ * No need to muck with permissions if bs is active.
+ * TODO: should activation be a separate function?
+ */
+assert(qemu_in_main_thread() || bdrv_is_active(bs));
+


I don’t understand this, really.  It looks to me like “if you don’t call 
this in the main thread, this better be a no-op”, i.e., you must never 
call this function in an I/O thread if you really want to use it.  I.e. 
what I’d classify as a GS function.


It sounds like this is just a special case for said test, and 
special-casing code for tests sounds like a bad idea.


Hanna


  QLIST_FOREACH(child, >children, next) {
  bdrv_co_invalidate_cache(child->bs, _err);
  if (local_err) {

Re: [PATCH v3 00/28] glib: Replace g_memdup() by g_memdup2()

[Laurent Vivier]

Le 15/12/2021 à 17:54, Philippe Mathieu-Daudé a écrit :

Hi Laurent,

On 9/3/21 19:44, Philippe Mathieu-Daudé wrote:


This series provides the safely equivalent g_memdup2() wrapper,
and replace all g_memdup() calls by it.



Philippe Mathieu-Daudé (28):
   hw/hyperv/vmbus: Remove unused vmbus_load/save_req()
   glib-compat: Introduce g_memdup2() wrapper
   qapi: Replace g_memdup() by g_memdup2()
   accel/tcg: Replace g_memdup() by g_memdup2()
   block/qcow2-bitmap: Replace g_memdup() by g_memdup2()
   softmmu: Replace g_memdup() by g_memdup2()
   hw/9pfs: Replace g_memdup() by g_memdup2()
   hw/acpi: Avoid truncating acpi_data_len() to 32-bit
   hw/acpi: Replace g_memdup() by g_memdup2()
   hw/core/machine: Replace g_memdup() by g_memdup2()
   hw/hppa/machine: Replace g_memdup() by g_memdup2()
   hw/i386/multiboot: Replace g_memdup() by g_memdup2()
   hw/net/eepro100: Replace g_memdup() by g_memdup2()
   hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2()
   hw/scsi/mptsas: Replace g_memdup() by g_memdup2()
   hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2()
   hw/rdma: Replace g_memdup() by g_memdup2()
   hw/vfio/pci: Replace g_memdup() by g_memdup2()
   hw/virtio: Replace g_memdup() by g_memdup2()
   net/colo: Replace g_memdup() by g_memdup2()
   ui/clipboard: Replace g_memdup() by g_memdup2()
   linux-user: Replace g_memdup() by g_memdup2()
   tests/unit: Replace g_memdup() by g_memdup2()
   tests/qtest: Replace g_memdup() by g_memdup2()
   target/arm: Replace g_memdup() by g_memdup2()
   target/ppc: Replace g_memdup() by g_memdup2()
   contrib: Replace g_memdup() by g_memdup2()
   checkpatch: Do not allow deprecated g_memdup(

Is it possible to take the reviewed patches 2, 24 and 28
via qemu-trivial, so the rest can be merged slowly by
each submaintainer?




Done for these 3 patches.

Thanks,
Laurent

Re: [PATCH v3 28/28] checkpatch: Do not allow deprecated g_memdup()

[Laurent Vivier]

Le 03/09/2021 à 19:45, Philippe Mathieu-Daudé a écrit :

g_memdup() is insecure and as been deprecated in GLib 2.68.
QEMU provides the safely equivalent g_memdup2() wrapper.

Do not allow more g_memdup() calls in the repository, provide
a hint to use g_memdup2().

Signed-off-by: Philippe Mathieu-Daudé 
---
  scripts/checkpatch.pl | 5 +
  1 file changed, 5 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb8eff233e0..5caa739db48 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2850,6 +2850,11 @@ sub process {
WARN("consider using g_path_get_$1() in preference to 
g_strdup($1())\n" . $herecurr);
}
  
+# enforce g_memdup2() over g_memdup()

+   if ($line =~ /\bg_memdup\s*\(/) {
+   ERROR("use g_memdup2() instead of unsafe g_memdup()\n" 
. $herecurr);
+   }
+
  # recommend qemu_strto* over strto* for numeric conversions
if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
ERROR("consider using qemu_$1 in preference to $1\n" . 
$herecurr);



Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH v3 24/28] tests/qtest: Replace g_memdup() by g_memdup2()

[Laurent Vivier]

Le 03/09/2021 à 19:45, Philippe Mathieu-Daudé a écrit :

Per 
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538

   The old API took the size of the memory to duplicate as a guint,
   whereas most memory functions take memory sizes as a gsize. This
   made it easy to accidentally pass a gsize to g_memdup(). For large
   values, that would lead to a silent truncation of the size from 64
   to 32 bits, and result in a heap area being returned which is
   significantly smaller than what the caller expects. This can likely
   be exploited in various modules to cause a heap buffer overflow.

Replace g_memdup() by the safer g_memdup2() wrapper.

Signed-off-by: Philippe Mathieu-Daudé 
---
  tests/qtest/libqos/ahci.c   | 6 +++---
  tests/qtest/libqos/qgraph.c | 2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index fba3e7a954e..eaa2096512e 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -639,8 +639,8 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
  AHCIOpts *opts;
  uint64_t buffer_in;
  
-opts = g_memdup((opts_in == NULL ? _opts : opts_in),

-sizeof(AHCIOpts));
+opts = g_memdup2((opts_in == NULL ? _opts : opts_in),
+ sizeof(AHCIOpts));
  
  buffer_in = opts->buffer;
  
@@ -860,7 +860,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)

  g_assert(!props->ncq || props->lba48);
  
  /* Defaults and book-keeping */

-cmd->props = g_memdup(props, sizeof(AHCICommandProp));
+cmd->props = g_memdup2(props, sizeof(AHCICommandProp));
  cmd->name = command_name;
  cmd->xbytes = props->size;
  cmd->prd_size = 4096;
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index d1dc4919305..109ff04e1e8 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -93,7 +93,7 @@ static void add_edge(const char *source, const char *dest,
  edge->type = type;
  edge->dest = g_strdup(dest);
  edge->edge_name = g_strdup(opts->edge_name ?: dest);
-edge->arg = g_memdup(opts->arg, opts->size_arg);
+edge->arg = g_memdup2(opts->arg, opts->size_arg);
  
  edge->before_cmd_line =

  opts->before_cmd_line ? g_strconcat(" ", opts->before_cmd_line, NULL) 
: NULL;



Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

[Laurent Vivier]

Le 03/09/2021 à 19:44, Philippe Mathieu-Daudé a écrit :

When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:

   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
   ...

g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538

Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").

Following this comment suggestion, implement the g_memdup2_qemu()
wrapper to g_memdup2(), and use the safer equivalent inlined when
we are using pre-2.68 GLib.

Reported-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 
---
  include/glib-compat.h | 37 +
  1 file changed, 37 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 9e95c888f54..8d01a8c01fb 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,43 @@
   * without generating warnings.
   */
  
+/*

+ * g_memdup2_qemu:
+ * @mem: (nullable): the memory to copy.
+ * @byte_size: the number of bytes to copy.
+ *
+ * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
+ * from @mem. If @mem is %NULL it returns %NULL.
+ *
+ * This replaces g_memdup(), which was prone to integer overflows when
+ * converting the argument from a #gsize to a #guint.
+ *
+ * This static inline version is a backport of the new public API from
+ * GLib 2.68, kept internal to GLib for backport to older stable releases.
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
+ *
+ * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
+ *  or %NULL if @mem is %NULL.
+ */
+static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
+{
+#if GLIB_CHECK_VERSION(2, 68, 0)
+return g_memdup2(mem, byte_size);
+#else
+gpointer new_mem;
+
+if (mem && byte_size != 0) {
+new_mem = g_malloc(byte_size);
+memcpy(new_mem, mem, byte_size);
+} else {
+new_mem = NULL;
+}
+
+return new_mem;
+#endif
+}
+#define g_memdup2(m, s) g_memdup2_qemu(m, s)
+
  #if defined(G_OS_UNIX)
  /*
   * Note: The fallback implementation is not MT-safe, and it returns a copy of



Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH-for-6.2 0/2] hw/scsi/megasas: Avoid overflowing the SGL buffer

[Paolo Bonzini]

On 11/19/21 21:11, Philippe Mathieu-Daudé wrote:

Fix issue #521 reported by Alex some months ago:
https://gitlab.com/qemu-project/qemu/-/issues/521

Philippe Mathieu-Daudé (2):
   hw/scsi/megasas: Fails command if SGL buffer overflows
   tests/qtest/fuzz-megasas-test: Add test for GitLab issue #521

  hw/scsi/megasas.c   |  1 +
  tests/qtest/fuzz-megasas-test.c | 30 ++
  2 files changed, 31 insertions(+)



Queued, thanks.

Paolo