| Message ID | 20220221180356.13527-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Further harden function pointers | expand |
On 21.02.2022 19:03, Andrew Cooper wrote: > Most IOMMU hooks are already altcall for performance reasons. Convert the > rest of them so we can harden all the hooks in Control Flow Integrity > configurations. This necessitates the use of iommu_{v,}call() in debug builds > too. > > Move the root iommu_ops from __read_mostly to __ro_after_init now that the > latter exists. There is no need for a forward declaration of vtd_ops any > more, meaning that __initconst_cf_clobber can be used for VTD and AMD. The connection between the forward declaration and the annotation addition isn't really clear to me. > --- a/xen/arch/x86/include/asm/iommu.h > +++ b/xen/arch/x86/include/asm/iommu.h > @@ -72,7 +72,6 @@ struct arch_iommu > > extern struct iommu_ops iommu_ops; > > -#ifdef NDEBUG > # include <asm/alternative.h> > # define iommu_call(ops, fn, args...) ({ \ > (void)(ops); \ > @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops; > (void)(ops); \ > alternative_vcall(iommu_ops.fn, ## args); \ > }) > -#endif > > static inline const struct iommu_ops *iommu_get_ops(void) > { > @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); > static inline int iommu_adjust_irq_affinities(void) > { > return iommu_ops.adjust_irq_affinities > - ? iommu_ops.adjust_irq_affinities() > + ? iommu_call(iommu_ops, adjust_irq_affinities) While this (and other instances below) is x86-only code, where - with the removal of the #ifdef above - we now know the first argument is always ignored, I think it would still better be of the correct type (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would better become "ASSERT((ops) == &iommu_ops)", which would check both type (compile time) and value (runtime). > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -540,7 +540,7 @@ int __init iommu_setup(void) > int iommu_suspend() > { > if ( iommu_enabled ) > - return iommu_get_ops()->suspend(); > + return iommu_call(iommu_get_ops(), suspend); This use of iommu_get_ops() in such constructs is a pattern we didn't have so far. Perhaps it just looks bogus, and all is fine in reality (apart from the whole idea being wrong for Arm, or really any environment where multiple dissimilar IOMMUs may be in use). Or wait, there are pre-existing cases (just not immediately visible when grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and iommu_setup_hpet_msi(). > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true; > > static unsigned int __read_mostly nr_iommus; > > -static struct iommu_ops vtd_ops; > static struct tasklet vtd_fault_tasklet; > > static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *); > @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) > return rc; > } > > -static struct iommu_ops __initdata vtd_ops = { > +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { Ah yes, the conversion to const (and the dropping of the forward decl) could have been part of "VT-d / x86: re-arrange cache syncing". With the missing &-s added and preferably with the description adjusted a little Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 22/02/2022 09:29, Jan Beulich wrote: > On 21.02.2022 19:03, Andrew Cooper wrote: >> Most IOMMU hooks are already altcall for performance reasons. Convert the >> rest of them so we can harden all the hooks in Control Flow Integrity >> configurations. This necessitates the use of iommu_{v,}call() in debug builds >> too. >> >> Move the root iommu_ops from __read_mostly to __ro_after_init now that the >> latter exists. There is no need for a forward declaration of vtd_ops any >> more, meaning that __initconst_cf_clobber can be used for VTD and AMD. > The connection between the forward declaration and the annotation addition > isn't really clear to me. > >> --- a/xen/arch/x86/include/asm/iommu.h >> +++ b/xen/arch/x86/include/asm/iommu.h >> @@ -72,7 +72,6 @@ struct arch_iommu >> >> extern struct iommu_ops iommu_ops; >> >> -#ifdef NDEBUG >> # include <asm/alternative.h> >> # define iommu_call(ops, fn, args...) ({ \ >> (void)(ops); \ >> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops; >> (void)(ops); \ >> alternative_vcall(iommu_ops.fn, ## args); \ >> }) >> -#endif >> >> static inline const struct iommu_ops *iommu_get_ops(void) >> { >> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); >> static inline int iommu_adjust_irq_affinities(void) >> { >> return iommu_ops.adjust_irq_affinities >> - ? iommu_ops.adjust_irq_affinities() >> + ? iommu_call(iommu_ops, adjust_irq_affinities) > While this (and other instances below) is x86-only code, where - with > the removal of the #ifdef above - we now know the first argument is > always ignored, I think it would still better be of the correct type > (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would > better become "ASSERT((ops) == &iommu_ops)", which would check both > type (compile time) and value (runtime). I'm happy to fold that change if you want. It ought to optimise out completely for being > >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -540,7 +540,7 @@ int __init iommu_setup(void) >> int iommu_suspend() >> { >> if ( iommu_enabled ) >> - return iommu_get_ops()->suspend(); >> + return iommu_call(iommu_get_ops(), suspend); > This use of iommu_get_ops() in such constructs is a pattern we didn't > have so far. Perhaps it just looks bogus, and all is fine in reality > (apart from the whole idea being wrong for Arm, or really any > environment where multiple dissimilar IOMMUs may be in use). Or wait, > there are pre-existing cases (just not immediately visible when > grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and > iommu_setup_hpet_msi(). I think this means your happy(ish) with the change? I agree that this is nonsense on ARM, but the codepath isn't used yet and someone's going to have to reconcile the conflicting views. >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true; >> >> static unsigned int __read_mostly nr_iommus; >> >> -static struct iommu_ops vtd_ops; >> static struct tasklet vtd_fault_tasklet; >> >> static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *); >> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) >> return rc; >> } >> >> -static struct iommu_ops __initdata vtd_ops = { >> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { > Ah yes, the conversion to const (and the dropping of the forward decl) > could have been part of "VT-d / x86: re-arrange cache syncing". > > With the missing &-s added and preferably with the description adjusted > a little > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
On 22/02/2022 10:54, Andrew Cooper wrote: > On 22/02/2022 09:29, Jan Beulich wrote: >> On 21.02.2022 19:03, Andrew Cooper wrote: >>> Most IOMMU hooks are already altcall for performance reasons. Convert the >>> rest of them so we can harden all the hooks in Control Flow Integrity >>> configurations. This necessitates the use of iommu_{v,}call() in debug builds >>> too. >>> >>> Move the root iommu_ops from __read_mostly to __ro_after_init now that the >>> latter exists. There is no need for a forward declaration of vtd_ops any >>> more, meaning that __initconst_cf_clobber can be used for VTD and AMD. >> The connection between the forward declaration and the annotation addition >> isn't really clear to me. >> >>> --- a/xen/arch/x86/include/asm/iommu.h >>> +++ b/xen/arch/x86/include/asm/iommu.h >>> @@ -72,7 +72,6 @@ struct arch_iommu >>> >>> extern struct iommu_ops iommu_ops; >>> >>> -#ifdef NDEBUG >>> # include <asm/alternative.h> >>> # define iommu_call(ops, fn, args...) ({ \ >>> (void)(ops); \ >>> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops; >>> (void)(ops); \ >>> alternative_vcall(iommu_ops.fn, ## args); \ >>> }) >>> -#endif >>> >>> static inline const struct iommu_ops *iommu_get_ops(void) >>> { >>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); >>> static inline int iommu_adjust_irq_affinities(void) >>> { >>> return iommu_ops.adjust_irq_affinities >>> - ? iommu_ops.adjust_irq_affinities() >>> + ? iommu_call(iommu_ops, adjust_irq_affinities) >> While this (and other instances below) is x86-only code, where - with >> the removal of the #ifdef above - we now know the first argument is >> always ignored, I think it would still better be of the correct type >> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would >> better become "ASSERT((ops) == &iommu_ops)", which would check both >> type (compile time) and value (runtime). > I'm happy to fold that change if you want. It ought to optimise out > completely for being Bah - sent too early. "for being tautological." ~Andrew
On 22.02.2022 11:54, Andrew Cooper wrote: > On 22/02/2022 09:29, Jan Beulich wrote: >> On 21.02.2022 19:03, Andrew Cooper wrote: >>> --- a/xen/drivers/passthrough/iommu.c >>> +++ b/xen/drivers/passthrough/iommu.c >>> @@ -540,7 +540,7 @@ int __init iommu_setup(void) >>> int iommu_suspend() >>> { >>> if ( iommu_enabled ) >>> - return iommu_get_ops()->suspend(); >>> + return iommu_call(iommu_get_ops(), suspend); >> This use of iommu_get_ops() in such constructs is a pattern we didn't >> have so far. Perhaps it just looks bogus, and all is fine in reality >> (apart from the whole idea being wrong for Arm, or really any >> environment where multiple dissimilar IOMMUs may be in use). Or wait, >> there are pre-existing cases (just not immediately visible when >> grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and >> iommu_setup_hpet_msi(). > > I think this means your happy(ish) with the change? Yes. It looks a little odd, but since we have precedents this ought to be fine. Jan
On 22.02.2022 12:02, Andrew Cooper wrote: > On 22/02/2022 10:54, Andrew Cooper wrote: >> On 22/02/2022 09:29, Jan Beulich wrote: >>> On 21.02.2022 19:03, Andrew Cooper wrote: >>>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); >>>> static inline int iommu_adjust_irq_affinities(void) >>>> { >>>> return iommu_ops.adjust_irq_affinities >>>> - ? iommu_ops.adjust_irq_affinities() >>>> + ? iommu_call(iommu_ops, adjust_irq_affinities) >>> While this (and other instances below) is x86-only code, where - with >>> the removal of the #ifdef above - we now know the first argument is >>> always ignored, I think it would still better be of the correct type >>> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would >>> better become "ASSERT((ops) == &iommu_ops)", which would check both >>> type (compile time) and value (runtime). >> I'm happy to fold that change if you want. It ought to optimise out >> completely for being > > Bah - sent too early. "for being tautological." It's tautological here, but not everywhere. But imo the ASSERT() is good to have anyway, i.e. even if it leaves traces elsewhere in debug builds. Jan
On 22/02/2022 11:02, Andrew Cooper wrote: > On 22/02/2022 10:54, Andrew Cooper wrote: >> On 22/02/2022 09:29, Jan Beulich wrote: >>> On 21.02.2022 19:03, Andrew Cooper wrote: >>>> Most IOMMU hooks are already altcall for performance reasons. Convert the >>>> rest of them so we can harden all the hooks in Control Flow Integrity >>>> configurations. This necessitates the use of iommu_{v,}call() in debug builds >>>> too. >>>> >>>> Move the root iommu_ops from __read_mostly to __ro_after_init now that the >>>> latter exists. There is no need for a forward declaration of vtd_ops any >>>> more, meaning that __initconst_cf_clobber can be used for VTD and AMD. >>> The connection between the forward declaration and the annotation addition >>> isn't really clear to me. >>> >>>> --- a/xen/arch/x86/include/asm/iommu.h >>>> +++ b/xen/arch/x86/include/asm/iommu.h >>>> @@ -72,7 +72,6 @@ struct arch_iommu >>>> >>>> extern struct iommu_ops iommu_ops; >>>> >>>> -#ifdef NDEBUG >>>> # include <asm/alternative.h> >>>> # define iommu_call(ops, fn, args...) ({ \ >>>> (void)(ops); \ >>>> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops; >>>> (void)(ops); \ >>>> alternative_vcall(iommu_ops.fn, ## args); \ >>>> }) >>>> -#endif >>>> >>>> static inline const struct iommu_ops *iommu_get_ops(void) >>>> { >>>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); >>>> static inline int iommu_adjust_irq_affinities(void) >>>> { >>>> return iommu_ops.adjust_irq_affinities >>>> - ? iommu_ops.adjust_irq_affinities() >>>> + ? iommu_call(iommu_ops, adjust_irq_affinities) >>> While this (and other instances below) is x86-only code, where - with >>> the removal of the #ifdef above - we now know the first argument is >>> always ignored, I think it would still better be of the correct type >>> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would >>> better become "ASSERT((ops) == &iommu_ops)", which would check both >>> type (compile time) and value (runtime). >> I'm happy to fold that change if you want. It ought to optimise out >> completely for being > Bah - sent too early. "for being tautological." Sadly, it turns out it's not. $ ../scripts/bloat-o-meter -c xen-syms-before xen-syms-after add/remove: 0/0 grow/shrink: 13/0 up/down: 369/0 (369) Function old new delta pci_add_device 1352 1416 +64 pci_remove_device 716 761 +45 iommu_map 341 382 +41 iommu_do_pci_domctl 1666 1704 +38 iommu_unmap 276 310 +34 deassign_device 353 386 +33 iommu_free_pgtables 310 329 +19 iommu_iotlb_flush_all 181 199 +18 iommu_iotlb_flush 260 278 +18 iommu_hwdom_init 68 86 +18 iommu_domain_destroy 54 70 +16 iommu_lookup_page 53 67 +14 iommu_dump_page_tables 261 272 +11 Total: Before=2194756, After=2195125, chg +0.02% add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) Data old new delta Total: Before=1699384, After=1699384, chg +0.00% add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) RO Data old new delta Total: Before=0, After=0, chg +0.00% is the delta in debug builds, while $ ../scripts/bloat-o-meter -c xen-syms-before xen-syms-after add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-57 (-57) Function old new delta iommu_resume 34 16 -18 iommu_suspend 42 23 -19 iommu_crash_shutdown 66 46 -20 Total: Before=2112261, After=2112204, chg -0.00% add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) Data old new delta Total: Before=1709424, After=1709424, chg +0.00% add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) RO Data old new delta Total: Before=0, After=0, chg +0.00% is the delta in release builds. This is a little weird - it's because the ASSERT(), in release builds, short circuits the evaluation of its condition, meaning that the BUG_ON() inside iommu_get_ops() doesn't get emitted. Irritatingly, there's no way I can spot to do this check with a BUILD_BUG_ON(), which would reduce the impact on the debug builds too. ~Andrew
diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h index 8a96ba1f097f..a87f6d416252 100644 --- a/xen/arch/x86/include/asm/iommu.h +++ b/xen/arch/x86/include/asm/iommu.h @@ -72,7 +72,6 @@ struct arch_iommu extern struct iommu_ops iommu_ops; -#ifdef NDEBUG # include <asm/alternative.h> # define iommu_call(ops, fn, args...) ({ \ (void)(ops); \ @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops; (void)(ops); \ alternative_vcall(iommu_ops.fn, ## args); \ }) -#endif static inline const struct iommu_ops *iommu_get_ops(void) { @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); static inline int iommu_adjust_irq_affinities(void) { return iommu_ops.adjust_irq_affinities - ? iommu_ops.adjust_irq_affinities() + ? iommu_call(iommu_ops, adjust_irq_affinities) : 0; } @@ -122,7 +120,7 @@ int iommu_enable_x2apic(void); static inline void iommu_disable_x2apic(void) { if ( x2apic_enabled && iommu_ops.disable_x2apic ) - iommu_ops.disable_x2apic(); + iommu_vcall(iommu_ops, disable_x2apic); } int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma, diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index e57f555d00d1..4b59a4efe9b6 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) hd->arch.amd.paging_mode, 0, 0); } -static const struct iommu_ops __initconstrel _iommu_ops = { +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { .init = amd_iommu_domain_init, .hwdom_init = amd_iommu_hwdom_init, .quarantine_init = amd_iommu_quarantine_init, diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index e220fea72c2f..c6b2c384d1dd 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -540,7 +540,7 @@ int __init iommu_setup(void) int iommu_suspend() { if ( iommu_enabled ) - return iommu_get_ops()->suspend(); + return iommu_call(iommu_get_ops(), suspend); return 0; } @@ -548,7 +548,7 @@ int iommu_suspend() void iommu_resume() { if ( iommu_enabled ) - iommu_get_ops()->resume(); + iommu_vcall(iommu_get_ops(), resume); } int iommu_do_domctl( @@ -578,7 +578,8 @@ void iommu_crash_shutdown(void) return; if ( iommu_enabled ) - iommu_get_ops()->crash_shutdown(); + iommu_vcall(iommu_get_ops(), crash_shutdown); + iommu_enabled = false; #ifndef iommu_intremap iommu_intremap = iommu_intremap_off; diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 56968a06a100..6a65ba1d8271 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true; static unsigned int __read_mostly nr_iommus; -static struct iommu_ops vtd_ops; static struct tasklet vtd_fault_tasklet; static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *); @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) return rc; } -static struct iommu_ops __initdata vtd_ops = { +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { .init = intel_iommu_domain_init, .hwdom_init = intel_iommu_hwdom_init, .quarantine_init = intel_iommu_quarantine_init, diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index ad5f44e13d98..17c0fe555dd0 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -27,7 +27,7 @@ #include <asm/setup.h> const struct iommu_init_ops *__initdata iommu_init_ops; -struct iommu_ops __read_mostly iommu_ops; +struct iommu_ops __ro_after_init iommu_ops; bool __read_mostly iommu_non_coherent; enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full; @@ -129,7 +129,7 @@ int iommu_enable_x2apic(void) if ( !iommu_ops.enable_x2apic ) return -EOPNOTSUPP; - return iommu_ops.enable_x2apic(); + return iommu_call(iommu_ops, enable_x2apic); } void iommu_update_ire_from_apic(
Most IOMMU hooks are already altcall for performance reasons. Convert the rest of them so we can harden all the hooks in Control Flow Integrity configurations. This necessitates the use of iommu_{v,}call() in debug builds too. Move the root iommu_ops from __read_mostly to __ro_after_init now that the latter exists. There is no need for a forward declaration of vtd_ops any more, meaning that __initconst_cf_clobber can be used for VTD and AMD. 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> --- xen/arch/x86/include/asm/iommu.h | 6 ++---- xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +- xen/drivers/passthrough/iommu.c | 7 ++++--- xen/drivers/passthrough/vtd/iommu.c | 3 +-- xen/drivers/passthrough/x86/iommu.c | 4 ++-- 5 files changed, 10 insertions(+), 12 deletions(-)