| Message ID | 20220113005855.1180101-2-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dom0less PV drivers | expand |
On 13.01.2022 01:58, Stefano Stabellini wrote: > --- 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) The name of the function isn't really suitable for being non-static. Can't we fold its functionality back into evtchn_allocate_port() (or the other way around, depending on the perspective you want to take) in case the caller passes in port 0? (Btw., it is imo wrong for the loop over ports to start at 0, when it is part of the ABI that port 0 is always invalid. evtchn_init() also better wouldn't depend on it being the only party to successfully invoke the function getting back port 0.) Jan
On Thu, 13 Jan 2022, Jan Beulich wrote: > On 13.01.2022 01:58, Stefano Stabellini wrote: > > --- 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) > > The name of the function isn't really suitable for being non-static. > Can't we fold its functionality back into evtchn_allocate_port() (or > the other way around, depending on the perspective you want to take) > in case the caller passes in port 0? (Btw., it is imo wrong for the > loop over ports to start at 0, when it is part of the ABI that port > 0 is always invalid. evtchn_init() also better wouldn't depend on it > being the only party to successfully invoke the function getting back > port 0.) I agree that "get_free_port" is not a great name for a non-static function. Also, it should be noted that for the sake of this patch series I could just call evtchn_allocate_port(d, 1) given that it is the first evtchn to be allocated. So maybe, that's the best way forward? To address your specific suggestion, in my opinion it would be best to have two different functions to allocate a new port: - one with a specific evtchn_port_t port parameter - one without it (meaning: "I don't care about the number") Folding the functionality of "give me any number" when 0 is passed to evtchn_allocate_port() doesn't seem to be an improvement to the API to me. That said, I am still OK with making the suggested change if that's what you prefer. Another alternative is simply to rename "get_free_port" to "evtchn_allocate" (or something else to distinguish it from evtchn_allocate_port).
On 14.01.2022 02:20, Stefano Stabellini wrote: > On Thu, 13 Jan 2022, Jan Beulich wrote: >> On 13.01.2022 01:58, Stefano Stabellini wrote: >>> --- 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) >> >> The name of the function isn't really suitable for being non-static. >> Can't we fold its functionality back into evtchn_allocate_port() (or >> the other way around, depending on the perspective you want to take) >> in case the caller passes in port 0? (Btw., it is imo wrong for the >> loop over ports to start at 0, when it is part of the ABI that port >> 0 is always invalid. evtchn_init() also better wouldn't depend on it >> being the only party to successfully invoke the function getting back >> port 0.) > > I agree that "get_free_port" is not a great name for a non-static > function. Also, it should be noted that for the sake of this patch > series I could just call evtchn_allocate_port(d, 1) given that it is the > first evtchn to be allocated. So maybe, that's the best way forward? > > > To address your specific suggestion, in my opinion it would be best to > have two different functions to allocate a new port: > - one with a specific evtchn_port_t port parameter > - one without it (meaning: "I don't care about the number") > > Folding the functionality of "give me any number" when 0 is passed to > evtchn_allocate_port() doesn't seem to be an improvement to the API to > me. I view it the other way around - that way the function would actually start matching its name. So far it's more like marking a given port number as in use, rather than allocating. > That said, I am still OK with making the suggested change if that's what > you prefer. Given experience, hoping for others to voice an opinion isn't likely to become reality. Jan
Hi Stefano, On 13/01/2022 04:58, Stefano Stabellini wrote: > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > get_free_port will soon be used to allocate the xenstore event channel > for dom0less domains. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/common/event_channel.c | 2 +- > xen/include/xen/event.h | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > 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). Cheers,
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; @@ -1195,7 +1216,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); 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..85dcf1d0c4 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 */ +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom); + /* Allocate a specific event channel port. */ int evtchn_allocate_port(struct domain *d, unsigned int port);
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 *? Jan
Hi Jan, On 25/01/2022 08:22, 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 *? This is feasible with some tweaking. Which reminds me that I have a similar patch to what you describe: https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=560d656a9a792450530eeefd0d06cfd54dcd7685 This is doing more than what we need here as it takes care about restoring a port (for Live-Update). Note that They are forward port from 4.11 to unstable and untested on the latter. Cheers,
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.
Hi Stefano, 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. Cheers,
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:
On 25.01.2022 23:49, Stefano Stabellini wrote: > On Tue, 25 Jan 2022, Jan Beulich wrote: >> On 25.01.2022 02:10, Stefano Stabellini wrote: >>> @@ -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. As an alternative to Julien's suggestion the function could also simply be given a new boolean parameter indicating whether to bypass the XSM check. That would be more explicit than deriving from system state. Jan
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. 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. Jan
Hi, 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. > > Jan > [1] https://lore.kernel.org/xen-devel/bfd645cf42ef7786183be15c222ad04beed362c0.camel@xen.org/
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. But either option is totally fine by me, just let me know your preference. diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index b024119896..01996bd9d8 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -94,6 +94,8 @@ static always_inline int xsm_default_action( case XSM_PRIV: if ( is_control_domain(src) ) return 0; + if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE ) + return 0; return -EPERM; default: LINKER_BUG_ON(1);
Hi Stefano, 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(). > But either option is totally fine by > me, just let me know your preference. > > > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index b024119896..01996bd9d8 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -94,6 +94,8 @@ static always_inline int xsm_default_action( > case XSM_PRIV: > if ( is_control_domain(src) ) > return 0; > + if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE ) I would surround this with unlikely and possibly prevent speculation (Jan?). Cheers,
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. 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. That said I'm afraid it hasn't become clear to me why the XSM check needs bypassing here in the first place, and why this is acceptable from a security standpoint. >> --- a/xen/include/xsm/dummy.h >> +++ b/xen/include/xsm/dummy.h >> @@ -94,6 +94,8 @@ static always_inline int xsm_default_action( >> case XSM_PRIV: >> if ( is_control_domain(src) ) >> return 0; >> + if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE ) > > I would surround this with unlikely and possibly prevent speculation (Jan?). Unlikely - perhaps yes. Preventing speculation in principle also yes, but not at the expense of adding a 2nd LFENCE (besides the one in is_control_domain()). Yet open-coding is_control_domain() wouldn't be very nice either. But as per above I hope anyway we're not going to need to find a good solution here. Jan
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,
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);
On 28.01.2022 02:38, Stefano Stabellini wrote: > --- 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) Nit: I don't think "disable" expresses the behavior. May I suggest "skip" or "bypass" or some such? Or invert the boolean and name it "check_xsm" or even simply "xsm"? Jan
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) { int port; diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index 21c95e14fd..0b35d9d4d2 100644 --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -71,6 +71,9 @@ void evtchn_free(struct domain *d, struct evtchn *chn); /* Allocate a specific event channel port. */ int evtchn_allocate_port(struct domain *d, unsigned int port); +/* Get fix free event channel port */ +int get_free_port(struct domain *d); + /* Unmask a local event-channel port. */ int evtchn_unmask(unsigned int port);