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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, Sep 28, 2020 at 3:49 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.)

I'm not familiar with vIRQ-s, but I did a little bit of review.  A
vIRQ source is always Xen and its destination is a domain, correct?
They don't allow a data flow between domains, so maybe that is why
they weren't hooked originally?  Hmmm, even for non-XSM, there is no
restriction on binding the "dom0" vIRQ-s?  Like you say, maybe the
"dom0" vIRQ-s should be behind a permission check?

> 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).

Yes, that all seems good and reasonable.  With XSM/Flask you also need
the AVC entry for send to be lightweight.

It wouldn't help with the domain transition case, but you could run
the xsm send hooks at bind time to pre-populate the cache.  That would
still require avc code to bypass the memory allocation when holding a
lock in case the entry isn't found.  Your preallocation idea could be
generalized to have avc maintain a reserve of nodes for use when it
cannot allocate.  When it can allocate, it would refill the reserve in
addition to whatever regular allocation it would perform.  But if it's
only evtchn_send that needs special handling, then the complexity may
not be worth adding.

> > 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.

Thanks for the confirmation.  I tested the XSA patches before seeing
this patch, and it "worked".  Catching the issue now is definitely
better than a mysterious deadlock later.

Thanks,
Jason