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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 27/01/2022 12:07, Jan Beulich wrote:
> On 27.01.2022 10:51, Julien Grall wrote:
> > On 27/01/2022 01:50, Stefano Stabellini wrote:
> > > On Wed, 26 Jan 2022, Julien Grall wrote:
> > > > On 26/01/2022 07:30, Jan Beulich wrote:
> > > > > On 26.01.2022 02:03, Stefano Stabellini wrote:
> > > > > > Are you guys OK with something like this?
> > > > > With proper proof that this isn't going to regress anything else, maybe.
> > > > That's why I sugested to also gate with system_state < SYS_STATE_boot so we
> > > > reduce the potential regression (the use of hypercall should be limited at
> > > > boot).
> > > >
> > > > FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
> > > > (but in the context of Live-Update).
> > > >
> > > > > But ...
> > > > >
> > > > > > --- 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;
> > > > > ... my first question would be under what circumstances you might observe
> > > > > DOMID_XEN here and hence why this check is there.
> > > For simplicity I'll answer all the points here.
> > >
> > > I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
> > > but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
> > > a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
> > > of <). The patch appended below works without issues.
> > >
> > > Alternatively, I am also quite happy with Jan's suggestion of passing an
> > > extra parameter to evtchn_alloc_unbound, it could be:
> > >
> > > int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
> > >
> > > So that XSM is enabled by default.
> > >
> > > Adding the bool parameter to evtchn_alloc_unbound has the advantage of
> > > having only a very minor impact.
> > We will likely need similar approach for other hypercalls in the future
> > if we need to call them from Xen context (e.g. when restoring domain
> > state during Live-Update).
> >
> > So my preference would be to directly go with modifying the
> > xsm_default_action().
> How would this help when a real XSM policy is in use? Already in SILO
> mode I think you wouldn't get past the check, as the idle domain
> doesn't satisfy silo_mode_dom_check()'s use of is_control_domain().
> Actually I'm not even sure you'd get that far - I was under the
> impression that the domain at other side of the channel may not even
> be around at that time, in which case silo_evtchn_unbound() would
> return -ESRCH.
This would not help for real XSM policy. We would either need to bypass 
XSM completely or decide what to do depending on the mode (e.g. SILO, 
FLASK...).
> Additionally relaxing things at a central place like this one comes
> with the risk of relaxing too much. As said in reply to Stefano - if
> this is to be done, proof will need to be provided that this doesn't
> and won't permit operations which aren't supposed to be permitted. I
> for one would much prefer relaxation on a case-by-case basis.
The problem is you will end up to modify a lot of code. This will be 
quite tedious when the policy is likely going to be the same (e.g. if we 
are booting, then allow to use the hypercall).
So I think it makes more sense to do the modification at central place. 
That said, this is not strictly necessary for what Stefano needs. So I 
am OK if we go with local hack and deferred the debate to when a wider 
solution is needed.
Cheers,

--
Julien Grall