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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.11.2022 06:01, Andrew Cooper 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. 

I'm going to commit that change nevertheless. _If_ we decide to revert,
reverting this change as well is going to be easy enough.

Apart of this I can only fully support Jürgen's earlier reply, adding
that iirc it was in part you supporting and/or motivating the change
originally. It is perfectly fine for you to have changed your mind,
but it doesn't help at all if you indicate you did but don't say why.
Even for the change done here you left everyone guessing where the
problem would be that you did hint at in, afaict, merely a private
discussion with George originally. I'm glad it was enough of a hint
for Jürgen to spot the issue. But then ...

> The changes broke the kexec_op() ABI and this is a blocking regression
> vs 4.16.

... you repeat the same pattern here.

> There was one redeeming property of the series, and yet there was no
> discussion anywhere about function pointer casts.  But given that the
> premise was disputed to begin with, and the performance testing that
> stood an outside chance of countering the dispute was ignored, and
> /then/ that my objections were disregarded and the series committed
> without calling a vote, I have to say that I'm very displeased with how
> this went.

For there to be a need to vote, there needs to be an active discussion,
laying out all the issues, such that everyone who would eventually have
to vote can actually form an opinion. We were quite far from that point.

In fact, seeing the repeating pattern of you voicing objections without
then following up, in the "Progressing of pending patch series" design
session in September in Cambridge I did suggest that not followed-up on
objections should expire after a reasonable amount of time. Just giving a
vague "no" (not always quite as brief, but we had extreme cases) should
not be sufficient to block a patch or series indefinitely. This is still
only a suggestion of mine, not the least because it's not really clear to
me how to progress this into something more formal, but there were no
objections to such a model there nor in reply to the notes that were sent
afterwards.

Jan