Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 25 Jan 2022, Julien Grall wrote:
> On 25/01/2022 22:49, Stefano Stabellini wrote:
> > On Tue, 25 Jan 2022, Jan Beulich wrote:
> > > On 25.01.2022 02:10, Stefano Stabellini wrote:
> > > > On Sun, 23 Jan 2022, Julien Grall wrote:
> > > > > > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > > > > > index da88ad141a..5b0bcaaad4 100644
> > > > > > --- a/xen/common/event_channel.c
> > > > > > +++ b/xen/common/event_channel.c
> > > > > > @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d,
> > > > > > evtchn_port_t
> > > > > > port)
> > > > > >        return 0;
> > > > > >    }
> > > > > >    -static int get_free_port(struct domain *d)
> > > > > > +int get_free_port(struct domain *d)
> > > > > 
> > > > > I dislike the idea to expose get_free_port() (or whichever name we
> > > > > decide)
> > > > > because this can be easily misused.
> > > > > 
> > > > > In fact looking at your next patch (#3), you are misusing it as it is
> > > > > meant to
> > > > > be called with d->event_lock. I know this doesn't much matter
> > > > > in your situation because this is done at boot with no other domains
> > > > > running
> > > > > (or potentially any event channel allocation). However, I still think
> > > > > we
> > > > > should get the API right.
> > > > > 
> > > > > I am also not entirely happy of open-coding the allocation in
> > > > > domain_build.c.
> > > > > Instead, I would prefer if we provide a new helper to allocate an
> > > > > unbound
> > > > > event channel. This would be similar to your v1 (I still need to
> > > > > review the
> > > > > patch though).
> > > > 
> > > > I am happy to go back to v1 and address feedback on that patch. However,
> > > > I am having difficulties with the implementation. Jan pointed out:
> > > > 
> > > > 
> > > > > > -
> > > > > > -    chn->state = ECS_UNBOUND;
> > > > > 
> > > > > This cannot be pulled ahead of the XSM check (or in general anything
> > > > > potentially resulting in an error), as check_free_port() relies on
> > > > > ->state remaining ECS_FREE until it is known that the calling function
> > > > > can't fail anymore.
> > > > 
> > > > This makes it difficult to reuse _evtchn_alloc_unbound for the
> > > > implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
> > > > to do it.
> > > > 
> > > > Instead, I just create a new public function called
> > > > "evtchn_alloc_unbound" and renamed the existing funtion to
> > > > "_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
> > > > static function should be the one starting with "_"). So the function
> > > > names are inverted compared to v1.
> > > > 
> > > > Please let me know if you have any better suggestions.
> > > > 
> > > > 
> > > > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > > > index da88ad141a..c6b7dd7fbd 100644
> > > > --- a/xen/common/event_channel.c
> > > > +++ b/xen/common/event_channel.c
> > > > @@ -18,6 +18,7 @@
> > > >     #include <xen/init.h>
> > > >   #include <xen/lib.h>
> > > > +#include <xen/err.h>
> > > >   #include <xen/errno.h>
> > > >   #include <xen/sched.h>
> > > >   #include <xen/irq.h>
> > > > @@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn
> > > > *chn)
> > > >       xsm_evtchn_close_post(chn);
> > > >   }
> > > >   -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > > > +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t
> > > > remote_dom)
> > > > +{
> > > > +    struct evtchn *chn;
> > > > +    int port;
> > > > +
> > > > +    if ( (port = get_free_port(d)) < 0 )
> > > > +        return ERR_PTR(port);
> > > > +    chn = evtchn_from_port(d, port);
> > > > +
> > > > +    evtchn_write_lock(chn);
> > > > +
> > > > +    chn->state = ECS_UNBOUND;
> > > > +    chn->u.unbound.remote_domid = remote_dom;
> > > > +    evtchn_port_init(d, chn);
> > > > +
> > > > +    evtchn_write_unlock(chn);
> > > > +
> > > > +    return chn;
> > > > +}
> > > > +
> > > > +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > > >   {
> > > >       struct evtchn *chn;
> > > >       struct domain *d;
> > > 
> > > Instead of introducing a clone of this function (with, btw, still
> > > insufficient locking), did you consider simply using the existing
> > > evtchn_alloc_unbound() as-is, i.e. with the caller passing
> > > evtchn_alloc_unbound_t *?
> > 
> > Yes, we tried that first. Unfortunately the (dummy) XSM check cannot
> > work. This is how we would want to call the function:
> > 
> > 
> >      alloc.dom = d->domain_id;
> >      alloc.remote_dom = hardware_domain->domain_id;
> >      rc = evtchn_alloc_unbound(&alloc);
> > 
> > 
> > This is the implementation of the XSM check:
> > 
> > static XSM_INLINE int xsm_evtchn_unbound(
> >      XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn, domid_t id2)
> > {
> >      XSM_ASSERT_ACTION(XSM_TARGET);
> >      return xsm_default_action(action, current->domain, d);
> > }
> > 
> > 
> > Note the usage of current->domain. If you have any suggestions on how to
> > fix it please let me know.
> 
> If I am not mistaken, current should still point to a domain (in this case
> idle).
> 
> So one alternative would be to ignore XSM if current->domain == idle and the
> system is booting (this could be part of xsm_default_action())
> 
> Another alternative would be to switch current to another domain. 'dom0'
> wouldn't be a solution because it doesn't exist for "true" dom0less. So a
> possibility would be to use dom_xen or create a fake build domain to be used
> for XSM check during boot.


Great suggestions! I went with the first suggestion above and confirmed
it solved the problem! With the appended patch I can just call the
current implementation of evtchn_alloc_unbound (made non-static) from
domain_build.c without any issues.

Are you guys OK with something like this?

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index b024119896..99c63ea8c5 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
             return 0;
         /* fall through */
     case XSM_PRIV:
-        if ( is_control_domain(src) )
+        if ( is_control_domain(src) ||
+             src->domain_id == DOMID_IDLE ||
+             src->domain_id == DOMID_XEN )
             return 0;
         return -EPERM;
     default: