xen/arch/x86/include/asm/paging.h | 5 ++--- xen/arch/x86/mm/hap/hap.c | 5 ++--- xen/arch/x86/mm/shadow/common.c | 2 +- xen/arch/x86/mm/shadow/multi.c | 17 ++++++++--------- 4 files changed, 13 insertions(+), 16 deletions(-)
Nicola reports that the XSA-438 fix introduced new MISRA violations because of
some incidental tidying it tried to do. The parameter is useless, so resolve
the MISRA regression by removing it.
hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
it to distinguish internal and external callers and therefore whether the
paging lock should be taken.
However, we have paging_lock_recursive() for this purpose, which also avoids
the ability for the shadow internal callers to accidentally not hold the lock.
Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow reference")
Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tim Deegan <tim@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
Slightly RFC. Only compile tested so far.
---
xen/arch/x86/include/asm/paging.h | 5 ++---
xen/arch/x86/mm/hap/hap.c | 5 ++---
xen/arch/x86/mm/shadow/common.c | 2 +-
xen/arch/x86/mm/shadow/multi.c | 17 ++++++++---------
4 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/include/asm/paging.h b/xen/arch/x86/include/asm/paging.h
index 8fad4cfc1823..f291f2f9a21f 100644
--- a/xen/arch/x86/include/asm/paging.h
+++ b/xen/arch/x86/include/asm/paging.h
@@ -118,8 +118,7 @@ struct paging_mode {
paddr_t ga, uint32_t *pfec,
unsigned int *page_order);
#endif
- pagetable_t (*update_cr3 )(struct vcpu *v, bool do_locking,
- bool noflush);
+ pagetable_t (*update_cr3 )(struct vcpu *v, bool noflush);
unsigned int guest_levels;
@@ -296,7 +295,7 @@ static inline bool paging_flush_tlb(const unsigned long *vcpu_bitmap)
* as the value to load into the host CR3 to schedule this vcpu */
static inline pagetable_t paging_update_cr3(struct vcpu *v, bool noflush)
{
- return paging_get_hostmode(v)->update_cr3(v, 1, noflush);
+ return paging_get_hostmode(v)->update_cr3(v, noflush);
}
/* Update all the things that are derived from the guest's CR0/CR3/CR4.
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index e30f543d2cc5..9f964c1d878f 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -707,8 +707,7 @@ static bool cf_check hap_invlpg(struct vcpu *v, unsigned long linear)
return 1;
}
-static pagetable_t cf_check hap_update_cr3(
- struct vcpu *v, bool do_locking, bool noflush)
+static pagetable_t cf_check hap_update_cr3(struct vcpu *v, bool noflush)
{
v->arch.hvm.hw_cr[3] = v->arch.hvm.guest_cr[3];
hvm_update_guest_cr3(v, noflush);
@@ -794,7 +793,7 @@ static void cf_check hap_update_paging_modes(struct vcpu *v)
}
/* CR3 is effectively updated by a mode change. Flush ASIDs, etc. */
- hap_update_cr3(v, 0, false);
+ hap_update_cr3(v, false);
unlock:
paging_unlock(d);
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 8211e77cc7ff..8aa7b698f879 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2510,7 +2510,7 @@ static void sh_update_paging_modes(struct vcpu *v)
}
#endif /* OOS */
- v->arch.paging.mode->update_cr3(v, 0, false);
+ v->arch.paging.mode->update_cr3(v, false);
}
/*
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 447512870d21..90cf0ceaa367 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2475,7 +2475,7 @@ static int cf_check sh_page_fault(
* In any case, in the PAE case, the ASSERT is not true; it can
* happen because of actions the guest is taking. */
#if GUEST_PAGING_LEVELS == 3
- v->arch.paging.mode->update_cr3(v, 0, false);
+ v->arch.paging.mode->update_cr3(v, false);
#else
ASSERT(d->is_shutting_down);
#endif
@@ -3156,17 +3156,13 @@ sh_update_linear_entries(struct vcpu *v)
sh_flush_local(d);
}
-static pagetable_t cf_check sh_update_cr3(struct vcpu *v, bool do_locking,
- bool noflush)
+static pagetable_t cf_check sh_update_cr3(struct vcpu *v, bool noflush)
/* Updates vcpu->arch.cr3 after the guest has changed CR3.
* Paravirtual guests should set v->arch.guest_table (and guest_table_user,
* if appropriate).
* HVM guests should also make sure hvm_get_guest_cntl_reg(v, 3) works;
* this function will call hvm_update_guest_cr(v, 3) to tell them where the
* shadow tables are.
- * If do_locking != 0, assume we are being called from outside the
- * shadow code, and must take and release the paging lock; otherwise
- * that is the caller's responsibility.
*/
{
struct domain *d = v->domain;
@@ -3184,7 +3180,11 @@ static pagetable_t cf_check sh_update_cr3(struct vcpu *v, bool do_locking,
return old_entry;
}
- if ( do_locking ) paging_lock(v->domain);
+ /*
+ * This is used externally (with the paging lock not taken) and internally
+ * by the shadow code (with the lock already taken).
+ */
+ paging_lock_recursive(v->domain);
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
/* Need to resync all the shadow entries on a TLB flush. Resync
@@ -3412,8 +3412,7 @@ static pagetable_t cf_check sh_update_cr3(struct vcpu *v, bool do_locking,
shadow_sync_other_vcpus(v);
#endif
- /* Release the lock, if we took it (otherwise it's the caller's problem) */
- if ( do_locking ) paging_unlock(v->domain);
+ paging_unlock(v->domain);
return old_entry;
}
base-commit: fb0ff49fe9f784bfee0370c2a3c5f20e39d7a1cb
--
2.30.2
On 20.09.2023 21:21, Andrew Cooper wrote:
> Nicola reports that the XSA-438 fix introduced new MISRA violations because of
> some incidental tidying it tried to do. The parameter is useless, so resolve
> the MISRA regression by removing it.
>
> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
> it to distinguish internal and external callers and therefore whether the
> paging lock should be taken.
>
> However, we have paging_lock_recursive() for this purpose, which also avoids
> the ability for the shadow internal callers to accidentally not hold the lock.
>
> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow reference")
> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>
> Slightly RFC. Only compile tested so far.
With shadow/none.c also suitably edited
Reviewed-by: Jan Beulich <jbeulich@suse.com>
I'm a little surprised you introduce new uses of the (kind of odd) recursive lock,
when previously you voiced your dislike for our use of such. ("Kind of odd" because
unlike spin_lock_recursive(), only the potentially inner caller needs to use the
recursive form of the acquire.)
Jan
Hi,
> On Sep 21, 2023, at 20:38, Jan Beulich <jbeulich@suse.com> wrote:
> On 20.09.2023 21:21, Andrew Cooper wrote:
>> Nicola reports that the XSA-438 fix introduced new MISRA violations because of
>> some incidental tidying it tried to do. The parameter is useless, so resolve
>> the MISRA regression by removing it.
>>
>> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
>> it to distinguish internal and external callers and therefore whether the
>> paging lock should be taken.
>>
>> However, we have paging_lock_recursive() for this purpose, which also avoids
>> the ability for the shadow internal callers to accidentally not hold the lock.
>>
>> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow reference")
>> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>
>> Slightly RFC. Only compile tested so far.
>
> With shadow/none.c also suitably edited
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Henry Wang <Henry.Wang@arm.com>
Kind regards,
Henry
On 21/09/2023 1:38 pm, Jan Beulich wrote:
> On 20.09.2023 21:21, Andrew Cooper wrote:
>> Nicola reports that the XSA-438 fix introduced new MISRA violations because of
>> some incidental tidying it tried to do. The parameter is useless, so resolve
>> the MISRA regression by removing it.
>>
>> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
>> it to distinguish internal and external callers and therefore whether the
>> paging lock should be taken.
>>
>> However, we have paging_lock_recursive() for this purpose, which also avoids
>> the ability for the shadow internal callers to accidentally not hold the lock.
>>
>> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow reference")
>> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>
>> Slightly RFC. Only compile tested so far.
> With shadow/none.c also suitably edited
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Ah yes - I did forget about none.c. Thanks.
> I'm a little surprised you introduce new uses of the (kind of odd) recursive lock,
> when previously you voiced your dislike for our use of such. ("Kind of odd" because
> unlike spin_lock_recursive(), only the potentially inner caller needs to use the
> recursive form of the acquire.)
I do very much dislike recursive locks, and I do think that an
alternative universe without them would be better code. But a stream of
int/bool params are a similarly bad antipattern too.
As paging_lock_recursive() is used for this exact purpose elsewhere,
it's silly not to use fix one of the problems when it doesn't really
make the other problem worse.
~Andrew
© 2016 - 2024 Red Hat, Inc.