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

From: Julien Grall <julien@xen.org>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Cc: "Jan Beulich" <JBeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>
Subject: Re: [PATCH v3 01/70] xen/sort: Switch to an extern inline implementation
Date: Tue, 22 Feb 2022 15:52:06 +0000	[thread overview]
Message-ID: <74edb8fc-3103-78eb-6877-2d2d6bdfa86f@xen.org> (raw)
In-Reply-To: <20220222152645.8844-2-andrew.cooper3@citrix.com>

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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

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@amazon.com>

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

Cheers,

-- 
Julien Grall