[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.