Re: [PATCH v6 18/23] xen/riscv: implement IRQ routing for device passthrough

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/23/26 3:30 PM, Jan Beulich wrote:
> On 20.07.2026 17:59, Oleksii Kurochko wrote:
> > +/* Route an IRQ to a specific guest */
> > +int route_irq_to_guest(struct domain *d, unsigned int virq,
> > +                       unsigned int irq, const char *devname)
> > +{
> > +    struct irqaction *action;
> > +    struct irq_guest *info;
> > +    struct irq_desc *desc;
> > +    unsigned long flags;
> > +    int retval = 0;
> > +
> > +    if ( d->is_dying )
> > +        return -EINVAL;
> > +
> > +    desc = irq_to_desc(irq);
> > +
> > +    /*
> > +     * release_irq() frees this action via xvfree(), relying on action
> > +     * being the first member of struct irq_guest so that &info->action
> > +     * coincides with info itself. Guard the layout so a future field
> > +     * reorder can't silently turn that into a free() of a mid-allocation
> > +     * pointer.
> > +     */
> > +    BUILD_BUG_ON(offsetof(struct irq_guest, action) != 0);
> Can't release_irq() simply use container_of()? One way or another it feels
> like you're painting yourself into a particular corner ...
If it isn't the best option then it is needed to follow they way we had 
before:
-/*
- * Describe an IRQ assigned to a guest.
- *
- * The irqaction is embedded here (rather than allocated separately with
- * its dev_id pointing at a standalone struct irq_guest) so that freeing
- * the action in release_irq() also frees this whole structure in one go.
- * That avoids the alternative of release_irq()'s caller having to free
- * dev_id itself (something like in Arm release_guest_irq()).
- */
+/* Describe an IRQ assigned to a guest */
 struct irq_guest
 {
-    struct irqaction action;
     struct domain *d;
     unsigned int virq;
 };
@@ -263,7 +254,6 @@ static struct irq_guest *irq_get_guest_info(struct 
irq_desc *desc)
     return desc->action->dev_id;
 }

-
 void release_irq(unsigned int irq, const void *dev_id)
 {
     struct irq_desc *desc;
@@ -361,6 +351,7 @@ int release_guest_irq(struct domain *d, unsigned int 
virq)
     spin_unlock_irqrestore(&desc->lock, flags);

     release_irq(desc->irq, info);
+    xvfree(info);

     return 0;

@@ -384,23 +375,20 @@ int route_irq_to_guest(struct domain *d, unsigned 
int virq,
     desc = irq_to_desc(irq);

-    /*
-     * release_irq() frees this action via xvfree(), relying on action
-     * being the first member of struct irq_guest so that &info->action
-     * coincides with info itself. Guard the layout so a future field
-     * reorder can't silently turn that into a free() of a mid-allocation
-     * pointer.
-     */
-    BUILD_BUG_ON(offsetof(struct irq_guest, action) != 0);
+    action = xvmalloc(struct irqaction);
+    if ( !action )
+        return -ENOMEM;

     info = xvmalloc(struct irq_guest);
     if ( !info )
+    {
+        xvfree(action);
         return -ENOMEM;
+    }

     info->d = d;
     info->virq = virq;

-    action = &info->action;
     action->dev_id = info;
     action->name = devname;
     action->free_on_release = true;
@@ -454,13 +442,15 @@ int route_irq_to_guest(struct domain *d, unsigned 
int virq,
     if ( retval )
     {
         release_irq(desc->irq, info);
-        return retval;
+        goto free_info;
     }

     return 0;

  out:
     spin_unlock_irqrestore(&desc->lock, flags);
+    xvfree(action);
+ free_info:
     xvfree(info);

     return retval;

I see an an item in changelog which meniotned that:
```
Drop xfree(info) from release_guest_irq() to avoid a potential
dangling-pointer issue with the ->dev_id field. Now that
struct irqaction action;' is embedded into 'struct irq_guest',
'info' will be freed as part of release_irq() at the end.
```

But it seems I don't see now why it will be dangled-pointer here as
info is referenced by exactly one pointer, action->dev_id, and nothing 
caches it: the only reader, irq_get_guest_info(), dereferences 
desc->action->dev_id under desc->lock and doesn't outlive the critical 
section. xvfree(info) only ever runs after release_irq() has unlinked 
the action from desc->action under the lock and waited for 
_IRQ_INPROGRESS to clear, so by then no CPU can reach ->dev_id. A 
concurrent release_guest_irq() for the same IRQ is excluded by clearing 
_IRQ_GUEST under desc->lock, and live unrouting of a running domain is 
rejected with -EBUSY. In the error paths, out: frees an action 
_setup_irq() never installed, and the intc_route_irq_to_guest() failure 
path calls release_irq() before freeing info.
So it seems like it is safe to use what we had in v4.

Any concerns?

~ Oleksii