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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3518 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]