[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v2 2/5] xen: export get_free_port
On Thu, 27 Jan 2022, Julien Grall wrote: > 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. OK, thank you. I'll go with the following then. diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index da88ad141a..dde85444fe 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -284,7 +284,7 @@ void evtchn_free(struct domain *d, struct evtchn *chn) xsm_evtchn_close_post(chn); } -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm) { struct evtchn *chn; struct domain *d; @@ -301,9 +301,12 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) ERROR_EXIT_DOM(port, d); chn = evtchn_from_port(d, port); - rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom); - if ( rc ) - goto out; + if ( !disable_xsm ) + { + rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom); + if ( rc ) + goto out; + } evtchn_write_lock(chn); @@ -1195,7 +1198,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) struct evtchn_alloc_unbound alloc_unbound; if ( copy_from_guest(&alloc_unbound, arg, 1) != 0 ) return -EFAULT; - rc = evtchn_alloc_unbound(&alloc_unbound); + rc = evtchn_alloc_unbound(&alloc_unbound, false); if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) ) rc = -EFAULT; /* Cleaning up here would be a mess! */ break; diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index 21c95e14fd..5ea3ac345b 100644 --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -68,6 +68,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest); /* Free an event channel. */ void evtchn_free(struct domain *d, struct evtchn *chn); +/* Create a new event channel port */ +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm); + /* Allocate a specific event channel port. */ int evtchn_allocate_port(struct domain *d, unsigned int port);
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |