Message ID | 9f8d0c4d-dec2-0175-09df-51d5e11c88e1@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86: mostly shadow related XSA-319 follow-up | expand |
On 15.07.2020 11:59, Jan Beulich wrote: > The filling and cleaning up of v->arch.guest_table in new_guest_cr3() > was apparently inconsistent so far: There was a type ref acquired > unconditionally for the new top level page table, but the dropping of > the old type ref was conditional upon !paging_mode_refcounts(). Mirror > this also to arch_set_info_guest(). > > Also move new_guest_cr3()'s #ifdef to around the function - both callers > now get built only when CONFIG_PV, i.e. no need to retain a stub. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> While I've got an ack from Tim, I think I need either an ack from Andrew or someone's R-b in order to commit this. Thanks, Jan > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1122,8 +1122,6 @@ int arch_set_info_guest( > > if ( !cr3_page ) > rc = -EINVAL; > - else if ( paging_mode_refcounts(d) ) > - /* nothing */; > else if ( cr3_page == v->arch.old_guest_table ) > { > v->arch.old_guest_table = NULL; > @@ -1144,8 +1142,7 @@ int arch_set_info_guest( > case -ERESTART: > break; > case 0: > - if ( !compat && !VM_ASSIST(d, m2p_strict) && > - !paging_mode_refcounts(d) ) > + if ( !compat && !VM_ASSIST(d, m2p_strict) ) > fill_ro_mpt(cr3_mfn); > break; > default: > @@ -1166,7 +1163,7 @@ int arch_set_info_guest( > > if ( !cr3_page ) > rc = -EINVAL; > - else if ( !paging_mode_refcounts(d) ) > + else > { > rc = get_page_type_preemptible(cr3_page, PGT_root_page_table); > switch ( rc ) > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -3149,9 +3149,9 @@ int vcpu_destroy_pagetables(struct vcpu > return rc; > } > > +#ifdef CONFIG_PV > int new_guest_cr3(mfn_t mfn) > { > -#ifdef CONFIG_PV > struct vcpu *curr = current; > struct domain *d = curr->domain; > int rc; > @@ -3220,7 +3220,7 @@ int new_guest_cr3(mfn_t mfn) > > pv_destroy_ldt(curr); /* Unconditional TLB flush later. */ > > - if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) ) > + if ( !VM_ASSIST(d, m2p_strict) ) > fill_ro_mpt(mfn); > curr->arch.guest_table = pagetable_from_mfn(mfn); > update_cr3(curr); > @@ -3231,30 +3231,24 @@ int new_guest_cr3(mfn_t mfn) > { > struct page_info *page = mfn_to_page(old_base_mfn); > > - if ( paging_mode_refcounts(d) ) > - put_page(page); > - else > - switch ( rc = put_page_and_type_preemptible(page) ) > - { > - case -EINTR: > - case -ERESTART: > - curr->arch.old_guest_ptpg = NULL; > - curr->arch.old_guest_table = page; > - curr->arch.old_guest_table_partial = (rc == -ERESTART); > - rc = -ERESTART; > - break; > - default: > - BUG_ON(rc); > - break; > - } > + switch ( rc = put_page_and_type_preemptible(page) ) > + { > + case -EINTR: > + case -ERESTART: > + curr->arch.old_guest_ptpg = NULL; > + curr->arch.old_guest_table = page; > + curr->arch.old_guest_table_partial = (rc == -ERESTART); > + rc = -ERESTART; > + break; > + default: > + BUG_ON(rc); > + break; > + } > } > > return rc; > -#else > - ASSERT_UNREACHABLE(); > - return -EINVAL; > -#endif > } > +#endif > > #ifdef CONFIG_PV > static int vcpumask_to_pcpumask( > >
On 31/07/2020 15:58, Jan Beulich wrote: > On 15.07.2020 11:59, Jan Beulich wrote: >> The filling and cleaning up of v->arch.guest_table in new_guest_cr3() >> was apparently inconsistent so far: There was a type ref acquired >> unconditionally for the new top level page table, but the dropping of >> the old type ref was conditional upon !paging_mode_refcounts(). Mirror >> this also to arch_set_info_guest(). >> >> Also move new_guest_cr3()'s #ifdef to around the function - both callers >> now get built only when CONFIG_PV, i.e. no need to retain a stub. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > While I've got an ack from Tim, I think I need either an ack from > Andrew or someone's R-b in order to commit this. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1122,8 +1122,6 @@ int arch_set_info_guest( if ( !cr3_page ) rc = -EINVAL; - else if ( paging_mode_refcounts(d) ) - /* nothing */; else if ( cr3_page == v->arch.old_guest_table ) { v->arch.old_guest_table = NULL; @@ -1144,8 +1142,7 @@ int arch_set_info_guest( case -ERESTART: break; case 0: - if ( !compat && !VM_ASSIST(d, m2p_strict) && - !paging_mode_refcounts(d) ) + if ( !compat && !VM_ASSIST(d, m2p_strict) ) fill_ro_mpt(cr3_mfn); break; default: @@ -1166,7 +1163,7 @@ int arch_set_info_guest( if ( !cr3_page ) rc = -EINVAL; - else if ( !paging_mode_refcounts(d) ) + else { rc = get_page_type_preemptible(cr3_page, PGT_root_page_table); switch ( rc ) --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -3149,9 +3149,9 @@ int vcpu_destroy_pagetables(struct vcpu return rc; } +#ifdef CONFIG_PV int new_guest_cr3(mfn_t mfn) { -#ifdef CONFIG_PV struct vcpu *curr = current; struct domain *d = curr->domain; int rc; @@ -3220,7 +3220,7 @@ int new_guest_cr3(mfn_t mfn) pv_destroy_ldt(curr); /* Unconditional TLB flush later. */ - if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) ) + if ( !VM_ASSIST(d, m2p_strict) ) fill_ro_mpt(mfn); curr->arch.guest_table = pagetable_from_mfn(mfn); update_cr3(curr); @@ -3231,30 +3231,24 @@ int new_guest_cr3(mfn_t mfn) { struct page_info *page = mfn_to_page(old_base_mfn); - if ( paging_mode_refcounts(d) ) - put_page(page); - else - switch ( rc = put_page_and_type_preemptible(page) ) - { - case -EINTR: - case -ERESTART: - curr->arch.old_guest_ptpg = NULL; - curr->arch.old_guest_table = page; - curr->arch.old_guest_table_partial = (rc == -ERESTART); - rc = -ERESTART; - break; - default: - BUG_ON(rc); - break; - } + switch ( rc = put_page_and_type_preemptible(page) ) + { + case -EINTR: + case -ERESTART: + curr->arch.old_guest_ptpg = NULL; + curr->arch.old_guest_table = page; + curr->arch.old_guest_table_partial = (rc == -ERESTART); + rc = -ERESTART; + break; + default: + BUG_ON(rc); + break; + } } return rc; -#else - ASSERT_UNREACHABLE(); - return -EINVAL; -#endif } +#endif #ifdef CONFIG_PV static int vcpumask_to_pcpumask(
The filling and cleaning up of v->arch.guest_table in new_guest_cr3() was apparently inconsistent so far: There was a type ref acquired unconditionally for the new top level page table, but the dropping of the old type ref was conditional upon !paging_mode_refcounts(). Mirror this also to arch_set_info_guest(). Also move new_guest_cr3()'s #ifdef to around the function - both callers now get built only when CONFIG_PV, i.e. no need to retain a stub. Signed-off-by: Jan Beulich <jbeulich@suse.com>