Message ID | 20190527092319.15844-1-stefanha@redhat.com |
---|---|
State | New |
Headers | show |
Series | block/linux-aio: explictly clear laiocb->co | expand |
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 >
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
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.
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;
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(+)