Re: Revert of the 4.17 hypercall handler changes Re: [PATCH-for-4.17] xen: fix generated code for calling hypercall handlers

[Juergen Gross <jgross@xxxxxxxx>]

On 09.11.22 21:16, George Dunlap wrote:
> > On 4 Nov 2022, at 05:01, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> >
> > On 03/11/2022 16:36, Juergen Gross wrote:
> > > The code generated for the call_handlers_*() macros needs to avoid
> > > undefined behavior when multiple handlers share the same priority.
> > > The issue is the hypercall number being unverified fed into the macros
> > > and then used to set a mask via "mask = 1ULL << <hypercall-number>".
> > >
> > > Avoid a shift amount of more than 63 by setting mask to zero in case
> > > the hypercall number is too large.
> > >
> > > Fixes: eca1f00d0227 ("xen: generate hypercall interface related code")
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > This is not a suitable fix.  There being a security issue is just the
> > tip of the iceberg.
> At the x86 Maintainer’s meeting on Monday, we (Andrew, Jan, and I) talked about 
> this patch.  Here is my summary of the conversation (with the caveat that I may get 
> some of the details wrong).
>
> The proposed benefits of the series are:
>
> 1. By removing indirect calls, it removes those as a “speculative attack 
> surface”.
>
> 2. By removing indirect calls, it provides some performance benefit, since 
> indirect calls  require an extra memory fetch.
>
> 3. It avoids casting function pointers to function pointers of a different 
> type.  Our current practice is *technically* UB, and is incompatible with some 
> hardware safety mechanisms which we may want to take advantage of at some point 
> in the future; the series addresses both.
>
> There were two incidental technical problems pointed out:
>
> 1. A potential shift of more than 64 bytes, which is UB; this has been fixed.
>
> 2. The prototype for the kexec_op call was changed from unsigned long to unsigned int; 
> this is an ABI change which will cause differing behavior.  Jan will be looking at how 
> he can fix this, now that it’s been noted.
>
> But the more fundamental costs include:
>
> 1. The code is much more difficult now to reason about
>
> 2. The code is much larger
The to be maintained code is smaller. The overall diffstat of the series shows
that more lines were deleted than added.

The generated code is larger, but this applies to other changes (new compiler,
modified build settings, ...) often enough, too.

> 3. The long if/else chain could theoretically help hypercalls at the top if the 
> chain, but would definitely begin to hurt hypercalls at the bottom of the 
> chain; and the more hypercalls we add, the more of a theoretical performance 
> penalty this will have
>
> 4. By using 64-bit masks, the implementation limits the number of hypercalls to 
> 64; a number we are likely to exceed if we implement ABIv2 to be compatible 
> with AMD SEV.
This is solvable at one central place.

> Additionally, there is a question about whether some of the alleged benefits 
> actually help:
>
> 1. On AMD processors, we enable IBRS, which completely removes indirect calls 
> as a speculative attack surface already.  And on Intel processors, this attack 
> surface has already been significantly reduced.  So removing indirect calls is 
> not as important an issue.
>
> 2. Normal branches are *also* a surface of speculative attacks; so even apart 
> from the above, all this series does is change one potential attack surface for 
> another one.
History has shown that speculative attacks via indirect branches are much
harder to solve. New ones coming up will probably have the same problem.

> 3. When we analyze theoretical performance with deep CPU pipelines and 
> speculation in mind, the theoretical disadvantage of indirect branches goes 
> away; and depending on the hardware, there is a theoretical performance 
> degradation.
>
> 4. From a practical perspective, the performance tests are very much insufficient to 
> show either that this is an improvement, or that does not cause a performance 
> regression.  To show that there hasn’t been a performance degradation, a 
> battery of tests needs to be done on hardware from a variety of different vendors and 
> cpu generations, since each of them will have different properties after all 
> speculative mitigations have been applied.
This argument is true for many changes we are doing.

The performance impact might be positive or negative. With the possibility
of priorities the impact can be controlled, though.

> So the argument is as follows:
>
> There is no speculative benefit for the series; there is insufficient 
> performance evidence, either to justify a performance benefit or to allay 
> doubts about a performance regression; and the benefit that there is 
> insufficient to counterbalance the costs, and so the series should be reverted.
>
> At the end of the discussion, Jan and I agreed that Andrew had made a good case 
> for the series to be removed at some point.  The discussion needs to be 
> concluded on the list, naturally; and if there is a consensus to remove the 
> series, the next question would be whether we should revert it now, before 
> 4.17.0, or wait until after the release and revert it then (perhaps with a 
> backport to 4.17.1).
>
> (Jan and Andy, please let me know if I’ve misunderstood anything from that 
> meeting.)
I'm not against reverting.

I just wanted to share my thoughts on above reasoning.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature