From mboxrd@z Thu Jan 1 00:00:00 1970 From: William Dauchy Subject: Re: [PATCH v2] kvm: fix waitqueue_active without memory barrier in virt/kvm/async_pf.c Date: Fri, 6 Nov 2015 14:02:46 +0100 Message-ID: <20151106130246.GG12546@gandi.net> References: <17EC94B0A072C34B8DCF0D30AD16044A02874DB5@BPXM09GP.gisp.nec.co.jp> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="eMnpOGXCMazMAbfp" Cc: Gleb Natapov , "kvm@vger.kernel.org" , Kosuke Tatsukawa To: Paolo Bonzini Return-path: Received: from mail4.gandi.net ([217.70.183.210]:48258 "EHLO gandi.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932824AbbKFNCu (ORCPT ); Fri, 6 Nov 2015 08:02:50 -0500 Content-Disposition: inline In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A02874DB5@BPXM09GP.gisp.nec.co.jp> Sender: kvm-owner@vger.kernel.org List-ID: --eMnpOGXCMazMAbfp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Paolo, Looking at the history of this function, is it reasonable to say it fixes the following commit? af585b9 KVM: Halt vcpu if page it tries to access is swapped out Does it make it a good candidate for -stable? Thanks, On Oct09 12:21, Kosuke Tatsukawa wrote: > async_pf_execute() seems to be missing a memory barrier which might > cause the waker to not notice the waiter and miss sending a wake_up as > in the following figure. >=20 > async_pf_execute kvm_vcpu_block > ------------------------------------------------------------------------ > spin_lock(&vcpu->async_pf.lock); > if (waitqueue_active(&vcpu->wq)) > /* The CPU might reorder the test for > the waitqueue up here, before > prior writes complete */ > prepare_to_wait(&vcpu->wq, &wait, > TASK_INTERRUPTIBLE); > /*if (kvm_vcpu_check_block(vcpu) < 0)= */ > /*if (kvm_arch_vcpu_runnable(vcpu)) = { */ > ... > return (vcpu->arch.mp_state =3D=3D = KVM_MP_STATE_RUNNABLE && > !vcpu->arch.apf.halted) > || !list_empty_careful(&vcpu->asy= nc_pf.done) > ... > return 0; > list_add_tail(&apf->link, > &vcpu->async_pf.done); > spin_unlock(&vcpu->async_pf.lock); > waited =3D true; > schedule(); > ------------------------------------------------------------------------ >=20 > The attached patch adds the missing memory barrier. >=20 > I found this issue when I was looking through the linux source code > for places calling waitqueue_active() before wake_up*(), but without > preceding memory barriers, after sending a patch to fix a similar > issue in drivers/tty/n_tty.c (Details about the original issue can be > found here: https://lkml.org/lkml/2015/9/28/849). >=20 > Signed-off-by: Kosuke Tatsukawa > --- > v2: > - Fixed comment based on feedback from Paolo > v1: > - https://lkml.org/lkml/2015/10/8/994 > --- > virt/kvm/async_pf.c | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) >=20 > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > index 44660ae..a0999d7 100644 > --- a/virt/kvm/async_pf.c > +++ b/virt/kvm/async_pf.c > @@ -94,6 +94,12 @@ static void async_pf_execute(struct work_struct *work) > =20 > trace_kvm_async_pf_completed(addr, gva); > =20 > + /* > + * Memory barrier is required here to make sure change to > + * vcpu->async_pf.done is visible from other CPUs. This memory > + * barrier pairs with prepare_to_wait's set_current_state() > + */ > + smp_mb(); > if (waitqueue_active(&vcpu->wq)) > wake_up_interruptible(&vcpu->wq); > =20 > --=20 > 1.7.1 --=20 William --eMnpOGXCMazMAbfp Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlY8pPYACgkQ1I6eqOUidQGG0QCgg4RW1OU8CkJAN96jCixB28ny MhAAoK+ddaWgxMZ2eg7i4a1e0jPt96BC =1dtP -----END PGP SIGNATURE----- --eMnpOGXCMazMAbfp--