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 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.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature