Am 22.11.2017 um 18:06 hat Stefan Hajnoczi geschrieben: > On Wed, Nov 22, 2017 at 07:33:28AM -0800, Deepa Srinivasan wrote: > > Starting qemu with the following arguments causes qemu to segfault: > > ... -device lsi,id=lsi0 -drive > > file=iscsi:<...>,format=raw,if=none,node-name= > > iscsi1 -device scsi-block,bus=lsi0.0,id=<...>,drive=iscsi1 > > > > This patch fixes blk_aio_ioctl() so it does not pass stack addresses to > > blk_aio_ioctl_entry() which may be invoked after blk_aio_ioctl() returns. > > More > > details about the bug follow. > > > > blk_aio_ioctl() invokes blk_aio_prwv() with blk_aio_ioctl_entry as the > > coroutine parameter. blk_aio_prwv() ultimately calls aio_co_enter(). > > > > When blk_aio_ioctl() is executed from within a coroutine context (e.g. > > iscsi_bh_cb()), aio_co_enter() adds the coroutine (blk_aio_ioctl_entry) to > > the current coroutine's wakeup queue. blk_aio_ioctl() then returns. > > > > When blk_aio_ioctl_entry() executes later, it accesses an invalid pointer: > > .... > > BlkRwCo *rwco = &acb->rwco; > > > > rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset, > > rwco->qiov->iov[0].iov_base); <--- qiov is > > invalid > > here > > ... > > > > In the case when blk_aio_ioctl() is called from a non-coroutine context, > > blk_aio_ioctl_entry() executes immediately. But if bdrv_co_ioctl() calls > > qemu_coroutine_yield(), blk_aio_ioctl() will return. When the coroutine > > execution is complete, control returns to blk_aio_ioctl_entry() after the > > call > > to blk_co_ioctl(). There is no invalid reference after this point, but the > > function is still holding on to invalid pointers. > > > > The fix is to allocate memory for the QEMUIOVector and struct iovec as part > > of > > the request struct which the IO buffer is part of. The memory for this > > struct is > > guaranteed to be valid till the AIO is completed. > > Thanks for the patch! > > AIO APIs currently don't require the caller to match qiov's lifetime to > the I/O request lifetime. This patch changes that for blk_aio_ioctl() > only. If we want to do this consistently then all aio callers need to > be audited and fixed. > > The alternative is to make the API copy qiov when necessary. That is > less efficient but avoids modifying all callers. > > Either way, the lifetime of qiov must be consistent across all aio APIs, > not just blk_aio_ioctl().
Don't all blk_aio_*() APIs that take a qiov pointer require that it remains valid until the request completes? I don't think they are copied anywhere for blk_aio_preadv/pwritev() before being passed to the block driver. So this does look consistent with the existing functions to me. Kevin
signature.asc
Description: PGP signature
