| Message ID | 1401787261-30166-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 03/06/2014 11:21, Stefan Hajnoczi ha scritto: > qemu_bh_schedule() is supposed to be thread-safe at least the first time > it is called. Unfortunately this is not quite true: > > bh->scheduled = 1; > aio_notify(bh->ctx); > > Since another thread may run the BH callback once it has been scheduled, > there is a race condition if the callback frees the BH before > aio_notify(bh->ctx) has a chance to run. > > Reported-by: Stefan Priebe <s.priebe@profihost.ag> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > async.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/async.c b/async.c > index 6930185..5b6fe6b 100644 > --- a/async.c > +++ b/async.c > @@ -117,15 +117,21 @@ void qemu_bh_schedule_idle(QEMUBH *bh) > > void qemu_bh_schedule(QEMUBH *bh) > { > + AioContext *ctx; > + > if (bh->scheduled) > return; > + ctx = bh->ctx; > bh->idle = 0; > - /* Make sure that idle & any writes needed by the callback are done > - * before the locations are read in the aio_bh_poll. > + /* Make sure that: > + * 1. idle & any writes needed by the callback are done before the > + * locations are read in the aio_bh_poll. > + * 2. ctx is loaded before scheduled is set and the callback has a chance > + * to execute. > */ > - smp_wmb(); > + smp_mb(); > bh->scheduled = 1; > - aio_notify(bh->ctx); > + aio_notify(ctx); > } > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On Tue, Jun 03, 2014 at 11:21:01AM +0200, Stefan Hajnoczi wrote: > qemu_bh_schedule() is supposed to be thread-safe at least the first time > it is called. Unfortunately this is not quite true: > > bh->scheduled = 1; > aio_notify(bh->ctx); > > Since another thread may run the BH callback once it has been scheduled, > there is a race condition if the callback frees the BH before > aio_notify(bh->ctx) has a chance to run. > > Reported-by: Stefan Priebe <s.priebe@profihost.ag> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > async.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) Applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
Tested-by: Stefan Priebe <s.priebe@profihost.ag> Am 03.06.2014 11:21, schrieb Stefan Hajnoczi: > qemu_bh_schedule() is supposed to be thread-safe at least the first time > it is called. Unfortunately this is not quite true: > > bh->scheduled = 1; > aio_notify(bh->ctx); > > Since another thread may run the BH callback once it has been scheduled, > there is a race condition if the callback frees the BH before > aio_notify(bh->ctx) has a chance to run. > > Reported-by: Stefan Priebe <s.priebe@profihost.ag> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > async.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/async.c b/async.c > index 6930185..5b6fe6b 100644 > --- a/async.c > +++ b/async.c > @@ -117,15 +117,21 @@ void qemu_bh_schedule_idle(QEMUBH *bh) > > void qemu_bh_schedule(QEMUBH *bh) > { > + AioContext *ctx; > + > if (bh->scheduled) > return; > + ctx = bh->ctx; > bh->idle = 0; > - /* Make sure that idle & any writes needed by the callback are done > - * before the locations are read in the aio_bh_poll. > + /* Make sure that: > + * 1. idle & any writes needed by the callback are done before the > + * locations are read in the aio_bh_poll. > + * 2. ctx is loaded before scheduled is set and the callback has a chance > + * to execute. > */ > - smp_wmb(); > + smp_mb(); > bh->scheduled = 1; > - aio_notify(bh->ctx); > + aio_notify(ctx); > } > > >
On Tue, Jun 03, 2014 at 02:52:40PM +0200, Stefan Priebe - Profihost AG wrote:
> Tested-by: Stefan Priebe <s.priebe@profihost.ag>
Thanks!
diff --git a/async.c b/async.c index 6930185..5b6fe6b 100644 --- a/async.c +++ b/async.c @@ -117,15 +117,21 @@ void qemu_bh_schedule_idle(QEMUBH *bh) void qemu_bh_schedule(QEMUBH *bh) { + AioContext *ctx; + if (bh->scheduled) return; + ctx = bh->ctx; bh->idle = 0; - /* Make sure that idle & any writes needed by the callback are done - * before the locations are read in the aio_bh_poll. + /* Make sure that: + * 1. idle & any writes needed by the callback are done before the + * locations are read in the aio_bh_poll. + * 2. ctx is loaded before scheduled is set and the callback has a chance + * to execute. */ - smp_wmb(); + smp_mb(); bh->scheduled = 1; - aio_notify(bh->ctx); + aio_notify(ctx); }
qemu_bh_schedule() is supposed to be thread-safe at least the first time it is called. Unfortunately this is not quite true: bh->scheduled = 1; aio_notify(bh->ctx); Since another thread may run the BH callback once it has been scheduled, there is a race condition if the callback frees the BH before aio_notify(bh->ctx) has a chance to run. Reported-by: Stefan Priebe <s.priebe@profihost.ag> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- async.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)