Re: [PATCH] xen/CET: Fix __initconst_cf_clobber

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 03/03/2022 07:35, Jan Beulich wrote:
> On 02.03.2022 23:10, Andrew Cooper wrote:
>> The linker script collecting .init.rodata.* ahead of .init.rodata.cf_clobber
>> accidentally causes __initconst_cf_clobber to be a no-op.
>>
>> Rearrange the linker script to unbreak this.
>>
>> The IOMMU adjust_irq_affinities() hooks currently violate the safety
>> requirement for being cf_clobber, by also being __initcall().
>>
>> Consolidate to a single initcall using iommu_call() (satisfying the 
>> cf_clobber
>> safety requirement), and also removes the dubious property that we'd call 
>> into
>> both vendors IOMMU drivers on boot, relying on the for_each_*() loops to be
>> empty for safety.
>>
>> With this fixed, an all-enabled build of Xen has 1681 endbr64's (1918
>> including .init.text) with 382 (23%) being clobbered during boot.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> This will do for the immediate purpose, so:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -210,6 +210,12 @@ SECTIONS
>>    DECL_SECTION(.init.data) {
>>  #endif
>>  
>> +       . = ALIGN(POINTER_ALIGN);
>> +       __initdata_cf_clobber_start = .;
>> +       *(.init.data.cf_clobber)
>> +       *(.init.rodata.cf_clobber)
>> +       __initdata_cf_clobber_end = .;
>> +
>>         *(.init.rodata)
>>         *(.init.rodata.*)
> I wonder if this shouldn't really be two sections. Live-patching will
> need to supply two ranges to apply_alternatives() anyway (one for each
> section, unless you want to start requiring to pass a linker script to
> "$(LD) -r" when generating live patches, just to fold the two sections),
> so in the core hypervisor we may want to follow suit.

I don't see why livepatches would need two sections - they're linked in
a similar way to Xen IIRC.  Either way, if changes are needed, they
should be part of the livepatch work.

>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -462,6 +462,12 @@ bool arch_iommu_use_permitted(const struct domain *d)
>>              likely(!p2m_get_hostp2m(d)->global_logdirty));
>>  }
>>  
>> +static int cf_check __init adjust_irq_affinities(void)
>> +{
>> +    return iommu_call(&iommu_ops, adjust_irq_affinities);
>> +}
>> +__initcall(adjust_irq_affinities);
> I assume it is intentional that you didn't re-use the inline wrapper,
> to avoid its (then non-__init) instantiation to stay with an ENDBR.
> Yet then you could at least _call_ that wrapper here, instead of open-
> coding it.

No - that was unintentional.  I only merged the initcalls late during
development and forgot the wrapper.

I've adjusted to:

-    return iommu_call(&iommu_ops, adjust_irq_affinities);
+    return iommu_adjust_irq_affinities();


> And I further think the iommu_enabled checks should move out
> of the vendor functions, plus the hook also has no need anymore to have
> a return type of int. I guess I'll make a follow-on patch if you don't
> want to fold this in here.

Yeah, I'd prefer not to fold cleanup into this bugfix, but there are
certainly improvements to be done.

~Andrew