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

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