| Message ID | 20160311184514.2768.97728.stgit@bahia.huguette.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/11/2016 07:48 PM, Greg Kurz wrote: > Since commit "60253ed1e6ec rng: add request queue support to rng-random", > the use of a spapr_rng device may hang vCPU threads. > > The following path is taken without holding the lock to the main loop mutex: > > h_random() > rng_backend_request_entropy() > rng_random_request_entropy() > qemu_set_fd_handler() > > The consequence is that entropy_available() may be called before the vCPU > thread could even queue the request: depending on the scheduling, it may > happen that entropy_available() does not call random_recv()->qemu_sem_post(). > The vCPU thread will then sleep forever in h_random()->qemu_sem_wait(). > > This could not happen before 60253ed1e6ec because entropy_available() used > to call random_recv() unconditionally. > > This patch ensures the lock is held to avoid the race. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> Reviewed-by: Cédric Le Goater <clg@fr.ibm.com> Thanks for finding this race, C. > --- > > Thomas, > > This is the problem mentioned by Cedric in: > > https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02526.html > > Cheers. > > -- > Greg > > hw/ppc/spapr_rng.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c > index a39d472b66fd..02d6be49f58e 100644 > --- a/hw/ppc/spapr_rng.c > +++ b/hw/ppc/spapr_rng.c > @@ -77,13 +77,13 @@ static target_ulong h_random(PowerPCCPU *cpu, sPAPRMachineState *spapr, > hrdata.val.v64 = 0; > hrdata.received = 0; > > - qemu_mutex_unlock_iothread(); > while (hrdata.received < 8) { > rng_backend_request_entropy(rngstate->backend, 8 - hrdata.received, > random_recv, &hrdata); > + qemu_mutex_unlock_iothread(); > qemu_sem_wait(&hrdata.sem); > + qemu_mutex_lock_iothread(); > } > - qemu_mutex_lock_iothread(); > > qemu_sem_destroy(&hrdata.sem); > args[0] = hrdata.val.v64; >
On 11.03.2016 19:48, Greg Kurz wrote: > Since commit "60253ed1e6ec rng: add request queue support to rng-random", > the use of a spapr_rng device may hang vCPU threads. > > The following path is taken without holding the lock to the main loop mutex: > > h_random() > rng_backend_request_entropy() > rng_random_request_entropy() > qemu_set_fd_handler() > > The consequence is that entropy_available() may be called before the vCPU > thread could even queue the request: depending on the scheduling, it may > happen that entropy_available() does not call random_recv()->qemu_sem_post(). > The vCPU thread will then sleep forever in h_random()->qemu_sem_wait(). > > This could not happen before 60253ed1e6ec because entropy_available() used > to call random_recv() unconditionally. > > This patch ensures the lock is held to avoid the race. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > > Thomas, > > This is the problem mentioned by Cedric in: > > https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02526.html > > Cheers. > > -- > Greg > > hw/ppc/spapr_rng.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c > index a39d472b66fd..02d6be49f58e 100644 > --- a/hw/ppc/spapr_rng.c > +++ b/hw/ppc/spapr_rng.c > @@ -77,13 +77,13 @@ static target_ulong h_random(PowerPCCPU *cpu, sPAPRMachineState *spapr, > hrdata.val.v64 = 0; > hrdata.received = 0; > > - qemu_mutex_unlock_iothread(); > while (hrdata.received < 8) { > rng_backend_request_entropy(rngstate->backend, 8 - hrdata.received, > random_recv, &hrdata); > + qemu_mutex_unlock_iothread(); > qemu_sem_wait(&hrdata.sem); > + qemu_mutex_lock_iothread(); > } > - qemu_mutex_lock_iothread(); > > qemu_sem_destroy(&hrdata.sem); > args[0] = hrdata.val.v64; Reviewed-by: Thomas Huth <thuth@redhat.com>
On Fri, Mar 11, 2016 at 07:48:47PM +0100, Greg Kurz wrote: > Since commit "60253ed1e6ec rng: add request queue support to rng-random", > the use of a spapr_rng device may hang vCPU threads. > > The following path is taken without holding the lock to the main loop mutex: > > h_random() > rng_backend_request_entropy() > rng_random_request_entropy() > qemu_set_fd_handler() > > The consequence is that entropy_available() may be called before the vCPU > thread could even queue the request: depending on the scheduling, it may > happen that entropy_available() does not call random_recv()->qemu_sem_post(). > The vCPU thread will then sleep forever in h_random()->qemu_sem_wait(). > > This could not happen before 60253ed1e6ec because entropy_available() used > to call random_recv() unconditionally. > > This patch ensures the lock is held to avoid the race. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> Applied to ppc-for-2.6 > --- > > Thomas, > > This is the problem mentioned by Cedric in: > > https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02526.html > > Cheers. >
diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c index a39d472b66fd..02d6be49f58e 100644 --- a/hw/ppc/spapr_rng.c +++ b/hw/ppc/spapr_rng.c @@ -77,13 +77,13 @@ static target_ulong h_random(PowerPCCPU *cpu, sPAPRMachineState *spapr, hrdata.val.v64 = 0; hrdata.received = 0; - qemu_mutex_unlock_iothread(); while (hrdata.received < 8) { rng_backend_request_entropy(rngstate->backend, 8 - hrdata.received, random_recv, &hrdata); + qemu_mutex_unlock_iothread(); qemu_sem_wait(&hrdata.sem); + qemu_mutex_lock_iothread(); } - qemu_mutex_lock_iothread(); qemu_sem_destroy(&hrdata.sem); args[0] = hrdata.val.v64;
Since commit "60253ed1e6ec rng: add request queue support to rng-random", the use of a spapr_rng device may hang vCPU threads. The following path is taken without holding the lock to the main loop mutex: h_random() rng_backend_request_entropy() rng_random_request_entropy() qemu_set_fd_handler() The consequence is that entropy_available() may be called before the vCPU thread could even queue the request: depending on the scheduling, it may happen that entropy_available() does not call random_recv()->qemu_sem_post(). The vCPU thread will then sleep forever in h_random()->qemu_sem_wait(). This could not happen before 60253ed1e6ec because entropy_available() used to call random_recv() unconditionally. This patch ensures the lock is held to avoid the race. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- Thomas, This is the problem mentioned by Cedric in: https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg02526.html Cheers. -- Greg hw/ppc/spapr_rng.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)