[Qemu-devel] [PATCH] block/linux-aio: explictly clear laiocb->co

[Qemu-devel] [PATCH] block/linux-aio: explictly clear laiocb->co

[Stefan Hajnoczi]

qemu_aio_get() does not zero allocated memory.  Explicitly initialize
laiocb->co to prevent an uninitialized memory access in
qemu_laio_process_completion().

Note that this bug has never manifested itself.  I guess we're lucky!

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
I challenge you to find a place where laiocb->co is initialized and then
we can drop this patch.  I've double-checked and cannot find it...

 block/linux-aio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/linux-aio.c b/block/linux-aio.c
index d4b61fb251..a097653be6 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -440,6 +440,7 @@ BlockAIOCB *laio_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
     int ret;
 
     laiocb = qemu_aio_get(&laio_aiocb_info, bs, cb, opaque);
+    laiocb->co = NULL;
     laiocb->nbytes = nb_sectors * BDRV_SECTOR_SIZE;
     laiocb->ctx = s;
     laiocb->ret = -EINPROGRESS;
-- 
2.21.0

Re: [Qemu-devel] [PATCH] block/linux-aio: explictly clear laiocb->co

[Kevin Wolf]

Am 27.05.2019 um 11:23 hat Stefan Hajnoczi geschrieben:
> qemu_aio_get() does not zero allocated memory.  Explicitly initialize
> laiocb->co to prevent an uninitialized memory access in
> qemu_laio_process_completion().
> 
> Note that this bug has never manifested itself.  I guess we're lucky!
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

That the bug never manifested itself might be because it's in an unused
function. How about we finally just remove the unused callback-based
laio_submit() from the code?

At the time when I converted linux-aio to coroutines, someone (maybe
Paolo?) insisted that we keep the old interface because we might add a
new user sometime with possible shortcuts that bypass the whole coroutine
path, but it hasn't happened and I think we've moved even further in the
opposite direction since then.

Kevin

> I challenge you to find a place where laiocb->co is initialized and then
> we can drop this patch.  I've double-checked and cannot find it...
> 
>  block/linux-aio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index d4b61fb251..a097653be6 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -440,6 +440,7 @@ BlockAIOCB *laio_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
>      int ret;
>  
>      laiocb = qemu_aio_get(&laio_aiocb_info, bs, cb, opaque);
> +    laiocb->co = NULL;
>      laiocb->nbytes = nb_sectors * BDRV_SECTOR_SIZE;
>      laiocb->ctx = s;
>      laiocb->ret = -EINPROGRESS;
> -- 
> 2.21.0
> 

Re: [Qemu-devel] [PATCH] block/linux-aio: explictly clear laiocb->co

[Paolo Bonzini]

On 30/05/19 10:42, Kevin Wolf wrote:
> Am 27.05.2019 um 11:23 hat Stefan Hajnoczi geschrieben:
>> qemu_aio_get() does not zero allocated memory.  Explicitly initialize
>> laiocb->co to prevent an uninitialized memory access in
>> qemu_laio_process_completion().
>>
>> Note that this bug has never manifested itself.  I guess we're lucky!
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> That the bug never manifested itself might be because it's in an unused
> function. How about we finally just remove the unused callback-based
> laio_submit() from the code?
> 
> At the time when I converted linux-aio to coroutines, someone (maybe
> Paolo?) insisted that we keep the old interface because we might add a
> new user sometime with possible shortcuts that bypass the whole coroutine
> path, but it hasn't happened and I think we've moved even further in the
> opposite direction since then.

Yes, I suppose it's time.  Spending time fixing bugs in dead code is
always a sign that it's time. :)

Paolo

Re: [Qemu-devel] [PATCH] block/linux-aio: explictly clear laiocb->co

[Julia Suvorova via Qemu-devel]

On 30.05.2019 17:07, Paolo Bonzini wrote:
> On 30/05/19 10:42, Kevin Wolf wrote:
>> Am 27.05.2019 um 11:23 hat Stefan Hajnoczi geschrieben:
>>> qemu_aio_get() does not zero allocated memory.  Explicitly initialize
>>> laiocb->co to prevent an uninitialized memory access in
>>> qemu_laio_process_completion().
>>>
>>> Note that this bug has never manifested itself.  I guess we're lucky!
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> That the bug never manifested itself might be because it's in an unused
>> function. How about we finally just remove the unused callback-based
>> laio_submit() from the code?
>>
>> At the time when I converted linux-aio to coroutines, someone (maybe
>> Paolo?) insisted that we keep the old interface because we might add a
>> new user sometime with possible shortcuts that bypass the whole coroutine
>> path, but it hasn't happened and I think we've moved even further in the
>> opposite direction since then.
> 
> Yes, I suppose it's time.  Spending time fixing bugs in dead code is
> always a sign that it's time. :)

Great, I'll clean it up.

Best regards, Julia Suvorova.