Re: [PATCH v8 1/9] xen/events: don't allow binding a global virq from any domain

[Jürgen Groß <jgross@xxxxxxxx>]

On 11.03.25 20:04, Julien Grall wrote:
> Hi Juergen,
>
> On 11/03/2025 09:51, Jürgen Groß wrote:
> > On 11.03.25 10:35, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 04/02/2025 11:33, Juergen Gross wrote:
> > > > Today Xen will happily allow binding a global virq by a domain which
> > > > isn't configured to receive it. This won't result in any bad actions,
> > > > but the bind will appear to have succeeded with no event ever being
> > > > received by that event channel.
> > > >
> > > > Instead of allowing the bind, error out if the domain isn't set to
> > > > handle that virq. Note that this check is inside the write_lock() on
> > > > purpose, as a future patch will put a related check into
> > > > set_global_virq_handler() with the addition of using the same lock.
> > >  > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > >
> > > I see this patch was already committed. But I have a question about the logic.
> > >
> > > > ---
> > > > V6:
> > > > - new patch
> > > > V7:
> > > > - move handling domain check inside locked region (Jan Beulich)
> > > > - style fix (Jan Beulich)
> > > > ---
> > > >   xen/common/event_channel.c | 21 +++++++++++++++++----
> > > >   1 file changed, 17 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > > > index 46281b16ce..cd6f5a1211 100644
> > > > --- a/xen/common/event_channel.c
> > > > +++ b/xen/common/event_channel.c
> > > > @@ -120,6 +120,13 @@ static uint8_t 
> > > > get_xen_consumer(xen_event_channel_notification_t fn)
> > > >   /* Get the notification function for a given Xen-bound event channel. */
> > > >   #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1])
> > > > +static struct domain *__read_mostly global_virq_handlers[NR_VIRQS];
> > > > +
> > > > +static struct domain *get_global_virq_handler(unsigned int virq)
> > > > +{
> > > > +    return global_virq_handlers[virq] ?: hardware_domain;
> > > > +}
> > > > +
> > > >   static bool virq_is_global(unsigned int virq)
> > > >   {
> > > >       switch ( virq )
> > > > @@ -469,6 +476,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> > > > evtchn_port_t port)
> > > >       struct domain *d = current->domain;
> > > >       int            virq = bind->virq, vcpu = bind->vcpu;
> > > >       int            rc = 0;
> > > > +    bool           is_global;
> > > >       if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
> > > >           return -EINVAL;
> > > > @@ -478,8 +486,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> > > > evtchn_port_t port)
> > > >       * speculative execution.
> > > >       */
> > > >       virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
> > > > +    is_global = virq_is_global(virq);
> > > > -    if ( virq_is_global(virq) && (vcpu != 0) )
> > > > +    if ( is_global && vcpu != 0 )
> > > >           return -EINVAL;
> > > >       if ( (v = domain_vcpu(d, vcpu)) == NULL )
> > > > @@ -487,6 +496,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> > > > evtchn_port_t port)
> > > >       write_lock(&d->event_lock);
> > > > +    if ( is_global && get_global_virq_handler(virq) != d )
> > > What prevent a race between get_global_virq_handler() and 
> > > set_global_virq_handler()? Also, it is not clear in the implementation of 
> > > get_global_virq_handler() that it will ever only read 
> > > global_virq_handlers[virq] once.
> > set_global_virq_handler() is taking the event_lock of the domain
> > registered as handler.
> >
> > So if a domain is registered for handling a virq, d->event_lock is
> > protecting against the handling domain to be changed. Concurrent
> > calls of set_global_virq_handler() are handled via taking the
> > global_virq_handlers_lock spin_lock.
> I agree this would work for evtchn_bind_virq() because we only ever compare. But 
> I still wonder whether get_global_virq_handler() should gain an ACCESS_ONCE()? 
> Could the compiler decide to read global_virq_handlers[...] twice and therefore 
> return NULL?
I don't think there is currently any use case of get_global_virq_handler()
where this would be a problem. get_global_virq_handler() is allowed to
return NULL in case of e.g. a dom0less system where hardware_domain is NULL.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature