From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751620AbdJFBcw (ORCPT ); Thu, 5 Oct 2017 21:32:52 -0400 Received: from mail-pg0-f68.google.com ([74.125.83.68]:36699 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751395AbdJFBcu (ORCPT ); Thu, 5 Oct 2017 21:32:50 -0400 X-Google-Smtp-Source: AOwi7QDc4t/rqLtS+2123Q8zGCXhPXaliyi3g7/OpIx888c3aswJ8QXl9r+cA6pWISQrQhwlac+FNw== Date: Fri, 6 Oct 2017 09:33:49 +0800 From: Boqun Feng To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Paul E. McKenney" , Peter Zijlstra , Wanpeng Li , Radim =?utf-8?B?S3LEjW3DocWZ?= , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org Subject: Re: [PATCH] kvm/x86: Avoid async PF to end RCU read-side critical section early in PREEMPT=n kernel Message-ID: <20171006013349.bgwpd3djojyxn7al@tardis> References: <20171003133653.1178-1-boqun.feng@gmail.com> <83be0b1a-24ee-28d2-3011-85c2c40e1442@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="y67dcvk47zommkpk" Content-Disposition: inline In-Reply-To: <83be0b1a-24ee-28d2-3011-85c2c40e1442@redhat.com> User-Agent: NeoMutt/20170912 (1.9.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --y67dcvk47zommkpk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 03, 2017 at 02:11:08PM +0000, Paolo Bonzini wrote: > I'd prefer a slight change in subject and topic: >=20 > ------- 8< -------- > Subject: [PATCH] kvm/x86: Avoid async PF preempting the kernel incorrectly >=20 > Currently, in PREEMPT_COUNT=3Dn kernel, kvm_async_pf_task_wait() could ca= ll > schedule() to reschedule in some cases. This could result in > accidentally ending the current RCU read-side critical section early, > causing random memory corruption in the guest, or otherwise preempting > the currently running task inside between preempt_disable and > preempt_enable. >=20 > The difficulty to handle this well is because we don't know whether an > async PF delivered in a preemptible section or RCU read-side critical sec= tion > for PREEMPT_COUNT=3Dn, since preempt_disable()/enable() and rcu_read_lock= /unlock() > are both no-ops in that case. >=20 > To cure this, we treat any async PF interrupting a kernel context as one > that cannot be preempted, preventing kvm_async_pf_task_wait() from choosi= ng > the schedule() path in that case. >=20 > To do so, a second parameter for kvm_async_pf_task_wait() is introduced, > so that we know whether it's called from a context interrupting the > kernel, and the parameter is set properly in all the callsites. >=20 > Cc: "Paul E. McKenney" > Cc: Peter Zijlstra > Cc: Wanpeng Li > Cc: stable@vger.kernel.org > Signed-off-by: Boqun Feng > ------- 8< -------- >=20 It's more concise and accurate now! Learned a lot from your modification of commit messages, thanks! Regards, Boqun > I'll let Radim pick up the patch. >=20 > Paolo >=20 > On 03/10/2017 15:36, Boqun Feng wrote: > > Currently, in PREEMPT=3Dn kernel, kvm_async_pf_task_wait() could call > > schedule() to reschedule in some cases, which could result in > > accidentally ending the current RCU read-side critical section early. > > And this could end up with random memory corruption in the guest. > >=20 > > The difficulty to handle this well is because we don't know whether an > > async PF delivered in a RCU read-side critical section for > > PREEMPT_COUNT=3Dn kernel, since rcu_read_lock/unlock() are just no-ops = in > > that case. > >=20 > > To cure this, we treat any async PF interrupting a kernel context as one > > delivered in a RCU read-side critical section, and we don't allow > > kvm_async_pf_task_wait() to choose schedule path in that case for > > PREEMPT_COUNT=3Dn kernel, because that will introduce unvolunteerly > > context switches and break the assumption for RCU to work properly. > >=20 > > To do so, a second parameter for kvm_async_pf_task_wait() is introduced, > > so that we know whether it's called from a context interrupting the > > kernel, and we set that parameter properly in all the callsites. > >=20 > > Cc: "Paul E. McKenney" > > Cc: Peter Zijlstra > > Cc: Wanpeng Li > > Signed-off-by: Boqun Feng > > --- > > arch/x86/include/asm/kvm_para.h | 4 ++-- > > arch/x86/kernel/kvm.c | 14 ++++++++++---- > > arch/x86/kvm/mmu.c | 2 +- > > 3 files changed, 13 insertions(+), 7 deletions(-) > >=20 > > diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm= _para.h > > index bc62e7cbf1b1..59ad3d132353 100644 > > --- a/arch/x86/include/asm/kvm_para.h > > +++ b/arch/x86/include/asm/kvm_para.h > > @@ -88,7 +88,7 @@ static inline long kvm_hypercall4(unsigned int nr, un= signed long p1, > > bool kvm_para_available(void); > > unsigned int kvm_arch_para_features(void); > > void __init kvm_guest_init(void); > > -void kvm_async_pf_task_wait(u32 token); > > +void kvm_async_pf_task_wait(u32 token, int interrupt_kernel); > > void kvm_async_pf_task_wake(u32 token); > > u32 kvm_read_and_reset_pf_reason(void); > > extern void kvm_disable_steal_time(void); > > @@ -103,7 +103,7 @@ static inline void kvm_spinlock_init(void) > > =20 > > #else /* CONFIG_KVM_GUEST */ > > #define kvm_guest_init() do {} while (0) > > -#define kvm_async_pf_task_wait(T) do {} while(0) > > +#define kvm_async_pf_task_wait(T, I) do {} while(0) > > #define kvm_async_pf_task_wake(T) do {} while(0) > > =20 > > static inline bool kvm_para_available(void) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > > index e675704fa6f7..8bb9594d0761 100644 > > --- a/arch/x86/kernel/kvm.c > > +++ b/arch/x86/kernel/kvm.c > > @@ -117,7 +117,11 @@ static struct kvm_task_sleep_node *_find_apf_task(= struct kvm_task_sleep_head *b, > > return NULL; > > } > > =20 > > -void kvm_async_pf_task_wait(u32 token) > > +/* > > + * @interrupt_kernel: Is this called from a routine which interrupts t= he kernel > > + * (other than user space)? > > + */ > > +void kvm_async_pf_task_wait(u32 token, int interrupt_kernel) > > { > > u32 key =3D hash_32(token, KVM_TASK_SLEEP_HASHBITS); > > struct kvm_task_sleep_head *b =3D &async_pf_sleepers[key]; > > @@ -140,8 +144,10 @@ void kvm_async_pf_task_wait(u32 token) > > =20 > > n.token =3D token; > > n.cpu =3D smp_processor_id(); > > - n.halted =3D is_idle_task(current) || preempt_count() > 1 || > > - rcu_preempt_depth(); > > + n.halted =3D is_idle_task(current) || > > + (IS_ENABLED(CONFIG_PREEMPT_COUNT) > > + ? preempt_count() > 1 || rcu_preempt_depth() > > + : interrupt_kernel); > > init_swait_queue_head(&n.wq); > > hlist_add_head(&n.link, &b->list); > > raw_spin_unlock(&b->lock); > > @@ -269,7 +275,7 @@ do_async_page_fault(struct pt_regs *regs, unsigned = long error_code) > > case KVM_PV_REASON_PAGE_NOT_PRESENT: > > /* page is swapped out by the host. */ > > prev_state =3D exception_enter(); > > - kvm_async_pf_task_wait((u32)read_cr2()); > > + kvm_async_pf_task_wait((u32)read_cr2(), !user_mode(regs)); > > exception_exit(prev_state); > > break; > > case KVM_PV_REASON_PAGE_READY: > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > > index eca30c1eb1d9..106d4a029a8a 100644 > > --- a/arch/x86/kvm/mmu.c > > +++ b/arch/x86/kvm/mmu.c > > @@ -3837,7 +3837,7 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, = u64 error_code, > > case KVM_PV_REASON_PAGE_NOT_PRESENT: > > vcpu->arch.apf.host_apf_reason =3D 0; > > local_irq_disable(); > > - kvm_async_pf_task_wait(fault_address); > > + kvm_async_pf_task_wait(fault_address, 0); > > local_irq_enable(); > > break; > > case KVM_PV_REASON_PAGE_READY: > >=20 >=20 --y67dcvk47zommkpk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlnW3XcACgkQSXnow7UH +rjyLgf/SErwG7PlZoVBOaZU3swqjxZjurpSK7GHPYIBJeO1GCTZ7zvoko0nnnvf Oqbe3RsEZdiZX24lTmiktqorIQbEk7N23R2rOwMAHpopj0o0H6Nb2bHAcMO2Cewp GbUaCIAjagYRtg5z6ecV+V8ceFtWknii///MPYkTxhXm05VGR2PAHm9ZZe1TwU4G OpTX8dqWo6dii0xeLc06uAFSvnT1kK0tpysfTaqTc4uVAN8aWBNUSgbVKC++DNPW v/b3pEk2O4f2IwQQQVLL3ATbEIOfF/vTC/2WLpzE+GPdxYJfA/Cfrh1+sMJFikH4 XPZCOUkHVmpZ9cICTBay0fTz6FBfwA== =NXUB -----END PGP SIGNATURE----- --y67dcvk47zommkpk--