| Message ID | 1528759585-51780-1-git-send-email-wangjie88@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] util/async: avoid NULL pointer dereference | expand |
On Tue, Jun 12, 2018 at 07:26:25AM +0800, Jie Wang wrote: > if laio_init create linux_aio failed and return NULL, NULL pointer > dereference will occur when laio_attach_aio_context dereference > linux_aio in aio_get_linux_aio. Let's avoid it and report error. > > Signed-off-by: Jie Wang <wangjie88@huawei.com> Reviewed-by: Jeff Cody <jcody@redhat.com> > --- > block/file-posix.c | 19 +++++++++++++++++-- > util/async.c | 5 ++++- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 513d371bb1..653017d7a5 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1665,6 +1665,11 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset, > #ifdef CONFIG_LINUX_AIO > } else if (s->use_linux_aio) { > LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); > + if (!aio) { > + s->use_linux_aio = false; > + error_report("Failed to get linux aio"); > + return -EIO; > + } > assert(qiov->size == bytes); > return laio_co_submit(bs, aio, s->fd, offset, qiov, type); > #endif > @@ -1695,7 +1700,12 @@ static void raw_aio_plug(BlockDriverState *bs) > BDRVRawState *s = bs->opaque; > if (s->use_linux_aio) { > LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); > - laio_io_plug(bs, aio); > + if (aio) { > + laio_io_plug(bs, aio); > + } else { > + s->use_linux_aio = false; > + error_report("Failed to get linux aio"); > + } > } > #endif > } > @@ -1706,7 +1716,12 @@ static void raw_aio_unplug(BlockDriverState *bs) > BDRVRawState *s = bs->opaque; > if (s->use_linux_aio) { > LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); > - laio_io_unplug(bs, aio); > + if (aio) { > + laio_io_unplug(bs, aio); > + } else { > + s->use_linux_aio = false; > + error_report("Failed to get linux aio"); > + } > } > #endif > } > diff --git a/util/async.c b/util/async.c > index 03f62787f2..08d71340f8 100644 > --- a/util/async.c > +++ b/util/async.c > @@ -327,8 +327,11 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx) > { > if (!ctx->linux_aio) { > ctx->linux_aio = laio_init(); > - laio_attach_aio_context(ctx->linux_aio, ctx); > + if (ctx->linux_aio) { > + laio_attach_aio_context(ctx->linux_aio, ctx); > + } > } > + > return ctx->linux_aio; > } > #endif > -- > 2.15.0.windows.1 > >
ping On 2018/6/12 11:32, Jeff Cody wrote: > On Tue, Jun 12, 2018 at 07:26:25AM +0800, Jie Wang wrote: >> if laio_init create linux_aio failed and return NULL, NULL pointer >> dereference will occur when laio_attach_aio_context dereference >> linux_aio in aio_get_linux_aio. Let's avoid it and report error. >> >> Signed-off-by: Jie Wang <wangjie88@huawei.com> > > Reviewed-by: Jeff Cody <jcody@redhat.com> > >> --- >> block/file-posix.c | 19 +++++++++++++++++-- >> util/async.c | 5 ++++- >> 2 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 513d371bb1..653017d7a5 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -1665,6 +1665,11 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset, >> #ifdef CONFIG_LINUX_AIO >> } else if (s->use_linux_aio) { >> LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); >> + if (!aio) { >> + s->use_linux_aio = false; >> + error_report("Failed to get linux aio"); >> + return -EIO; >> + } >> assert(qiov->size == bytes); >> return laio_co_submit(bs, aio, s->fd, offset, qiov, type); >> #endif >> @@ -1695,7 +1700,12 @@ static void raw_aio_plug(BlockDriverState *bs) >> BDRVRawState *s = bs->opaque; >> if (s->use_linux_aio) { >> LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); >> - laio_io_plug(bs, aio); >> + if (aio) { >> + laio_io_plug(bs, aio); >> + } else { >> + s->use_linux_aio = false; >> + error_report("Failed to get linux aio"); >> + } >> } >> #endif >> } >> @@ -1706,7 +1716,12 @@ static void raw_aio_unplug(BlockDriverState *bs) >> BDRVRawState *s = bs->opaque; >> if (s->use_linux_aio) { >> LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); >> - laio_io_unplug(bs, aio); >> + if (aio) { >> + laio_io_unplug(bs, aio); >> + } else { >> + s->use_linux_aio = false; >> + error_report("Failed to get linux aio"); >> + } >> } >> #endif >> } >> diff --git a/util/async.c b/util/async.c >> index 03f62787f2..08d71340f8 100644 >> --- a/util/async.c >> +++ b/util/async.c >> @@ -327,8 +327,11 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx) >> { >> if (!ctx->linux_aio) { >> ctx->linux_aio = laio_init(); >> - laio_attach_aio_context(ctx->linux_aio, ctx); >> + if (ctx->linux_aio) { >> + laio_attach_aio_context(ctx->linux_aio, ctx); >> + } >> } >> + >> return ctx->linux_aio; >> } >> #endif >> -- >> 2.15.0.windows.1 >> >> > > . >
On Tue, Jun 12, 2018 at 07:26:25AM +0800, Jie Wang wrote: > if laio_init create linux_aio failed and return NULL, NULL pointer > dereference will occur when laio_attach_aio_context dereference > linux_aio in aio_get_linux_aio. Let's avoid it and report error. > > Signed-off-by: Jie Wang <wangjie88@huawei.com> > --- > block/file-posix.c | 19 +++++++++++++++++-- > util/async.c | 5 ++++- > 2 files changed, 21 insertions(+), 3 deletions(-) If someone wants to split aio_get_linux_aio() into an initialization function and a "get" function which doesn't return NULL if init succeeded, then we can make this a bit cleaner. But it doesn't matter at the moment since there are few callers and duplicating the NULL check isn't too bad. Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
Am 18.06.2018 um 17:50 hat Stefan Hajnoczi geschrieben: > On Tue, Jun 12, 2018 at 07:26:25AM +0800, Jie Wang wrote: > > if laio_init create linux_aio failed and return NULL, NULL pointer > > dereference will occur when laio_attach_aio_context dereference > > linux_aio in aio_get_linux_aio. Let's avoid it and report error. > > > > Signed-off-by: Jie Wang <wangjie88@huawei.com> > > --- > > block/file-posix.c | 19 +++++++++++++++++-- > > util/async.c | 5 ++++- > > 2 files changed, 21 insertions(+), 3 deletions(-) > > If someone wants to split aio_get_linux_aio() into an initialization > function and a "get" function which doesn't return NULL if init > succeeded, then we can make this a bit cleaner. But it doesn't matter > at the moment since there are few callers and duplicating the NULL check > isn't too bad. > > Thanks, applied to my block tree: > https://github.com/stefanha/qemu/commits/block Did you see this patch? [RFC v2] aio: properly bubble up errors from initialization Message-Id: <20180615174729.20544-1-naravamudan@digitalocean.com> I didn't review it yet, but it seems to be for the same, or at least a similar, problem. Kevin
Ping... On 2018/6/12 7:26, Jie Wang wrote: > if laio_init create linux_aio failed and return NULL, NULL pointer > dereference will occur when laio_attach_aio_context dereference > linux_aio in aio_get_linux_aio. Let's avoid it and report error. > > Signed-off-by: Jie Wang <wangjie88@huawei.com> > --- > block/file-posix.c | 19 +++++++++++++++++-- > util/async.c | 5 ++++- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 513d371bb1..653017d7a5 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1665,6 +1665,11 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset, > #ifdef CONFIG_LINUX_AIO > } else if (s->use_linux_aio) { > LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); > + if (!aio) { > + s->use_linux_aio = false; > + error_report("Failed to get linux aio"); > + return -EIO; > + } > assert(qiov->size == bytes); > return laio_co_submit(bs, aio, s->fd, offset, qiov, type); > #endif > @@ -1695,7 +1700,12 @@ static void raw_aio_plug(BlockDriverState *bs) > BDRVRawState *s = bs->opaque; > if (s->use_linux_aio) { > LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); > - laio_io_plug(bs, aio); > + if (aio) { > + laio_io_plug(bs, aio); > + } else { > + s->use_linux_aio = false; > + error_report("Failed to get linux aio"); > + } > } > #endif > } > @@ -1706,7 +1716,12 @@ static void raw_aio_unplug(BlockDriverState *bs) > BDRVRawState *s = bs->opaque; > if (s->use_linux_aio) { > LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); > - laio_io_unplug(bs, aio); > + if (aio) { > + laio_io_unplug(bs, aio); > + } else { > + s->use_linux_aio = false; > + error_report("Failed to get linux aio"); > + } > } > #endif > } > diff --git a/util/async.c b/util/async.c > index 03f62787f2..08d71340f8 100644 > --- a/util/async.c > +++ b/util/async.c > @@ -327,8 +327,11 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx) > { > if (!ctx->linux_aio) { > ctx->linux_aio = laio_init(); > - laio_attach_aio_context(ctx->linux_aio, ctx); > + if (ctx->linux_aio) { > + laio_attach_aio_context(ctx->linux_aio, ctx); > + } > } > + > return ctx->linux_aio; > } > #endif >
Thanks Stefan, will you push it to master branch? On 2018/6/18 23:50, Stefan Hajnoczi wrote: > On Tue, Jun 12, 2018 at 07:26:25AM +0800, Jie Wang wrote: >> if laio_init create linux_aio failed and return NULL, NULL pointer >> dereference will occur when laio_attach_aio_context dereference >> linux_aio in aio_get_linux_aio. Let's avoid it and report error. >> >> Signed-off-by: Jie Wang <wangjie88@huawei.com> >> --- >> block/file-posix.c | 19 +++++++++++++++++-- >> util/async.c | 5 ++++- >> 2 files changed, 21 insertions(+), 3 deletions(-) > > If someone wants to split aio_get_linux_aio() into an initialization > function and a "get" function which doesn't return NULL if init > succeeded, then we can make this a bit cleaner. But it doesn't matter > at the moment since there are few callers and duplicating the NULL check > isn't too bad. > > Thanks, applied to my block tree: > https://github.com/stefanha/qemu/commits/block > > Stefan >
On Tue, Jun 26, 2018 at 10:51:50AM +0800, WangJie (Pluto) wrote:
> Thanks Stefan, will you push it to master branch?
Hi,
I have replaced this patch with "[PATCH v4] linux-aio: properly bubble
up errors from initialization". It solves the issue at initialization
time so aio_get_linux_aio() callers don't need to check for NULL.
Kevin pointed out the other patch. Sorry that work was duplicated, it
seems that both you and the other author worked on this bug at the same
time.
Please feel free to review "[PATCH v4] linux-aio: properly bubble up
errors from initialization" and reply on that email thread if you have
any concerns.
Stefan
diff --git a/block/file-posix.c b/block/file-posix.c index 513d371bb1..653017d7a5 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1665,6 +1665,11 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset, #ifdef CONFIG_LINUX_AIO } else if (s->use_linux_aio) { LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); + if (!aio) { + s->use_linux_aio = false; + error_report("Failed to get linux aio"); + return -EIO; + } assert(qiov->size == bytes); return laio_co_submit(bs, aio, s->fd, offset, qiov, type); #endif @@ -1695,7 +1700,12 @@ static void raw_aio_plug(BlockDriverState *bs) BDRVRawState *s = bs->opaque; if (s->use_linux_aio) { LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); - laio_io_plug(bs, aio); + if (aio) { + laio_io_plug(bs, aio); + } else { + s->use_linux_aio = false; + error_report("Failed to get linux aio"); + } } #endif } @@ -1706,7 +1716,12 @@ static void raw_aio_unplug(BlockDriverState *bs) BDRVRawState *s = bs->opaque; if (s->use_linux_aio) { LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs)); - laio_io_unplug(bs, aio); + if (aio) { + laio_io_unplug(bs, aio); + } else { + s->use_linux_aio = false; + error_report("Failed to get linux aio"); + } } #endif } diff --git a/util/async.c b/util/async.c index 03f62787f2..08d71340f8 100644 --- a/util/async.c +++ b/util/async.c @@ -327,8 +327,11 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx) { if (!ctx->linux_aio) { ctx->linux_aio = laio_init(); - laio_attach_aio_context(ctx->linux_aio, ctx); + if (ctx->linux_aio) { + laio_attach_aio_context(ctx->linux_aio, ctx); + } } + return ctx->linux_aio; } #endif
if laio_init create linux_aio failed and return NULL, NULL pointer dereference will occur when laio_attach_aio_context dereference linux_aio in aio_get_linux_aio. Let's avoid it and report error. Signed-off-by: Jie Wang <wangjie88@huawei.com> --- block/file-posix.c | 19 +++++++++++++++++-- util/async.c | 5 ++++- 2 files changed, 21 insertions(+), 3 deletions(-)