Re: [PATCH v3 01/70] xen/sort: Switch to an extern inline implementation

[Julien Grall <julien@xxxxxxx>]

Hi,

On 22/02/2022 15:26, Andrew Cooper wrote:
> There are exactly 3 callers of sort() in the hypervisor.  Callbacks in a tight
> loop like this are problematic for performance, especially with Spectre v2
> protections, which is why extern inline is used commonly by libraries.
>
> Both ARM callers pass in NULL for the swap function, and while this might seem
> like an attractive option at first, it causes generic_swap() to be used, which
> forced a byte-wise copy.  Provide real swap functions so the compiler can
> optimise properly, which is very important for ARM downstreams where
> milliseconds until the system is up matters.
After the previous discussion, I was expecting the sentence "Provide 
real..." to be completely dropped. Instead the change should be 
justified with...
> This is also important for Control Flow Integrity schemes (e.g. x86 CET-IBT,
> ARM BTI), because tagged function(s) performing an arbitrary length swap of
> two arbitrary pointers is a very valuable gadget for an attacker.
... this one as this is the real reason of the change. Not the 
performance (unless you have numbers proving it).
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
To be pedantic, my Nacked-by hasn't been yet revoked (see [1]). So you 
should have kept it in the new version.
Anyway, given that the patch makes sense for BTI, I am willing to 
replace the Nacked-by with an Acked-by:
Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

[1] 
https://lore.kernel.org/xen-devel/70824a0c-cc48-b064-695c-35c2d06c0ad1@xxxxxxx/
Cheers,

--
Julien Grall