Re: [PATCH v6 2/7] xen/events: don't allow binding a global virq from any domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2025 11:17, Juergen Gross wrote:
> --- 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 *global_virq_handlers[NR_VIRQS] __read_mostly;

Nit: While you move this line around, it would be nice if the attribute
could then also move to its canonical place (between type and identifier).

> +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 )
> @@ -479,8 +486,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> evtchn_port_t port)
>      */
>      virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
>  
> -    if ( virq_is_global(virq) && (vcpu != 0) )
> -        return -EINVAL;
> +    if ( virq_is_global(virq) )
> +    {
> +        if ( get_global_virq_handler(virq) != d )
> +            return -EBUSY;

Hmm. While this eliminates the problem for the common, race free case,
the handler changing right after the check would still mean the bind
would succeed.

Plus this way you're breaking a case that afaict has been working so
far: The bind happening before the setting of the handler. With a lot
of unrelated if-s and when-s this could e.g. be of interest when
considering a re-startable Xenstore domain. The one to take over could
start first, obtain state from the original one while that's still
active, and be nominated the handler of the global vIRQ only in the
last moment.

Jan