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@suse.com>
---
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 )
+    {
+        rc = -EBUSY;
+        goto out;
+    }
+
     if ( read_atomic(&v->virq_to_evtchn[virq]) )
     {
         rc = -EEXIST;
@@ -965,15 +980,13 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
     }
 }
 
-static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
-
 static DEFINE_SPINLOCK(global_virq_handlers_lock);
 
 void send_global_virq(uint32_t virq)
 {
     ASSERT(virq_is_global(virq));
 
-    send_guest_global_virq(global_virq_handlers[virq] ?: hardware_domain, virq);
+    send_guest_global_virq(get_global_virq_handler(virq), virq);
 }
 
 int set_global_virq_handler(struct domain *d, uint32_t virq)
-- 
2.43.0

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@suse.com>

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.

Cheers,

-- 
Julien Grall

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@suse.com>
> 
> 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

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@suse.com>
>>
>> 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

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@suse.com>
>>>
>>> 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