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

[Julien Grall <julien@xxxxxxx>]

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?
Cheers,

--
Julien Grall