Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2020 20:08, Jason Andryuk wrote:
> On Fri, Sep 25, 2020 at 12:02 PM Julien Grall <julien@xxxxxxx> wrote:
>>
>> Hi Jan,
>>
>> On 25/09/2020 16:36, Jan Beulich wrote:
>>> On 25.09.2020 16:33, Julien Grall wrote:
>>>> On 25/09/2020 14:58, Jan Beulich wrote:
>>>>> On 25.09.2020 15:16, Julien Grall wrote:
>>>>>> Fair enough. I would still like to consider a way where we could avoid
>>>>>> to hack xsm_* because we have the interrupts disabled.
>>>>>
>>>>> Well, from a conceptual pov it's at least questionable for XSM to
>>>>> need any memory allocations at all when merely being asked for
>>>>> permission. And indeed the need for it arises solely from its
>>>>> desire to cache the result, for the sake of subsequent lookups.
>>>>>
>>>>> I also find it odd that there's an XSM check on the send path in
>>>>> the first place. This isn't just because it would seem to me that
>>>>> it should be decided at binding time whether sending is permitted
>>>>> - I may easily be missing something in the conceptual model here.
>>>>> It's also because __domain_finalise_shutdown() too uses
>>>>> evtchn_send(), and I didn't think this one should be subject to
>>>>> any XSM check (just like send_guest_*() aren't).
>>>>
>>>> Maybe this is the first question we need to answer?
>>>
>>> Indeed that was the question I asked myself before putting together
>>> the patch. Yet I have no idea who could answer it, with Daniel
>>> having gone silent for quite long a time now. Hence I didn't even
>>> put up the question, but instead tried to find a halfway reasonable
>>> solution.
>>
>> IIRC, XSM is used by OpenXT and QubeOS. So I have CCed them to get some
>> input.
> 
> I think the send hook exists because send is its own distinct
> operation.  While most common usage could be handled by just checking
> at bind time, the send hoor provides more flexibility.  For instance,
> the send hook can be used to restrict signalling to only one
> direction.

I did realize this is a special case, but it could still be dealt
with at binding time, via a boolean in struct evtchn.

>  Also, a domain label can transition (change) at runtime.
> Dropping the send check would latch the permission at bind time which
> would not necessarily be valid for the security policy.

I did realize this as a possibility too, but there the immediate
question is: Why for interdomain channels, but then not also for
vIRQ-s, for example? In fact, unless I'm overlooking something,
for this specific case there's not even any check in the binding
logic, not even for global vIRQ-s. (After all there are two
aspects in the permissions here: One is to be eligible to send,
which ought to not matter when the sender is Xen, while the
other is permission to learn / know of certain events, i.e. in
particular global vIRQ-s.)

The fundamental issue here is that the sending path should be
fast and hence lightweight. This means (to me) that in
particular no memory allocations should occur, and (more
generally) no global or domain wide locks should be taken (for
rw ones: no write locks).

> Am I correct that the assertion mentioned in the patch description
> would only be seen in debug builds?

Yes. Release builds would instead have deadlock potential, which
may get realized at any time (or never, if you're really lucky).
Catching such cases early, and in an easy to recognize manner,
is - after all - what the extra checking in debug builds is for.

Jan