Re: [PATCH v2] x86/irq: Skip unmap_domain_pirq XSM during destruction

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2022 13:10, Roger Pau Monné wrote:
> On Thu, Apr 07, 2022 at 10:51:50AM -0400, Jason Andryuk wrote:
>> xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
>> complete_domain_destroy as an RCU callback.  The source context was an
>> unexpected, random domain.  Since this is a xen-internal operation,
>> going through the XSM hook is inapproriate.
>>
>> Check d->is_dying and skip the XSM hook when set since this is a cleanup
>> operation for a domain being destroyed.
>>
>> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
>> ---
>> v2:
>> Style fixes
>> Rely on ret=0 initialization
>>
>> ---
>>  xen/arch/x86/irq.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
>> index 285ac399fb..de30ee7779 100644
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -2340,8 +2340,14 @@ int unmap_domain_pirq(struct domain *d, int pirq)
>>          nr = msi_desc->msi.nvec;
>>      }
>>  
>> -    ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
>> -                               msi_desc ? msi_desc->dev : NULL);
>> +    /*
>> +     * When called by complete_domain_destroy via RCU, current is a random
>> +     * domain.  Skip the XSM check since this is a Xen-initiated action.
>> +     */
>> +    if ( !d->is_dying )
>> +        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
>> +                                   msi_desc ? msi_desc->dev : NULL);
>> +
> 
> Nit: I would remove the extra space here, but that's a question of
> taste...

Which extra space are you referring to? The only candidate I can spot
are the two adjacent spaces in the comment, between the two sentences.
But that's several lines up. And I think we have examples of both
single and double spaces in the code base for such cases. I know I'm
not even consistent myself in this regard - the longer a comment gets,
the more likely I am to use two spaces between sentences.

> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> I wonder if long term we could make this cleaner, maybe by moving the
> unbind so it always happen in the context of the caller of the destroy
> hypercall instead of in the RCU context?

This would be nice, but when I looked at this long ago it didn't seem
straightforward to achieve.

Jan