Re: [PATCH v4 21/25] xen/riscv: implement IRQ routing for device passthrough

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/3/26 9:21 AM, Jan Beulich wrote:
> On 02.07.2026 18:04, Oleksii Kurochko wrote:
> > On 7/2/26 4:32 PM, Jan Beulich wrote:
> > > On 02.07.2026 11:33, Oleksii Kurochko wrote:
> > > > On 7/2/26 8:38 AM, Jan Beulich wrote:
> > > > > > > > +    spin_unlock_irqrestore(&desc->lock, flags);
> > > > > > > > +
> > > > > > > > +    release_irq(desc->irq, info);
> > > > > > > > +    xvfree(info);
> > > > > > > If, in release_irq(), action isn't freed, it's ->dev_id field will now have
> > > > > > > a dangling pointer. (I think I did point this out before.)
> We'll be back to this if ...
>
> > > > > > It should freed in release_irq() as route_irq_to_guest() always set
> > > > > > action->free_on_release = true;
> > > > > Well, "free_on_release" must exits for a purpose. I.e. there must be, now
> > > > > or soon, cases where it's set to false. Else simply drop the field.
> > > > I can't simply remove this field because it is part of the common
> > > > `struct irqaction` and is used by other arc-s. (I assume that it is not
> > > > what you fully meant...)
> > > >
> > > > IIUC, this field is used to determine whether an irqaction is
> > > > heap-allocated (and therefore should be freed) or statically allocated
> > > > (and therefore should not be freed).
> > > Yes. However, all uses of the field are in arch-specific code. So in
> > > principle it could be #ifdef-ed out for RISC-V. There may be a better
> > > option, though:
> > >
> > > > At the moment, all IRQ actions are heap-allocated (on RISC-V), so
> > > > free_on_release should always be set to true. In particular, the code
> > > > snippet you asked about releases a guest interrupt, and guest interrupt
> > > > actions are always heap-allocated. As a result, when release_guest_irq()
> > > > calls release_irq(), the associated irqaction will be freed.
> > > >
> > > > So, from what I can see, the current behavior is correct and I think it
> > > > should be left as it is. Do you have any concerns about this?
> > > If you only ever have the field set to true, use ASSERT() in place of
> > > if().
> > It looks like that if we want an `ASSERT()`, it should be in
> > `release_guest_irq()`. However, that would require duplicating part of
> > `release_irq()` to locate the corresponding `irqaction` and verify
> > `free_on_release`. Since guest IRQs are created through
> > `route_irq_to_guest()`, which always sets `action->free_on_release =
> > true`, we are already safe.
> >
> > I don't see much benefit in replacing the `if` statement with an
> > `ASSERT()` in `release_irq()`. The current implementation is generic and
> > would also work for Xen-owned, statically allocated interrupts (even
> > though there are none today). If we replace the `if` with an `ASSERT()`
> > now, anyone introducing a Xen-owned, statically allocated interrupt in
> > the future would simply have to revert the change and restore the `if`
> > statement, bringing us back to the current implementation.
> ... such a change was made. Imo you have two choices: Deal with the issue
> properly right away, keeping the if().
So then my fix will look like as embed struct irqaction as the first 
member of struct irq_guest and allocate them as one block. Then 
xvfree(action) inside release_irq() (gated by the still-meaningful if 
(action->free_on_release)) frees the whole thing - no separate 
xvfree(info) needed anywhere, so the dangling pointer can't happen 
regardless of what free_on_release is:
$ git diff
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
index 830f8d5d5997..146156f1caae 100644
--- a/xen/arch/riscv/irq.c
+++ b/xen/arch/riscv/irq.c
@@ -19,9 +19,18 @@
 #include <asm/hardirq.h>
 #include <asm/intc.h>

-/* Describe an IRQ assigned to a guest */
+/*
+ * 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()).
+ */
 struct irq_guest
 {
+    struct irqaction action;
     struct domain *d;
     unsigned int virq;
 };
@@ -345,7 +354,6 @@ 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;

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

-    action = xvmalloc(struct irqaction);
-    if ( !action )
-        return -ENOMEM;
+    /*
+     * 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);

     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;
@@ -436,15 +447,13 @@ int route_irq_to_guest(struct domain *d, unsigned 
int virq,
     if ( retval )
     {
         release_irq(desc->irq, info);
-        goto free_info;
+        return retval;
     }

     return 0;

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

     return retval;

Also, with such approach release_guest_irq() could be dropped at all, at 
least for now.
Does this approach look good?

 Or assume "free_on_release" is only
> ever true, and add an assertion to this effect (indicating that code needs
> auditing if that assumption is broken).
I think I'm okay with using ASSERT() instead of the changes suggested above.

What I don't understand is why you think it would be better to put the 
ASSERT() in release_irq() instead of keeping the if():
```
...
    if ( action->free_on_release )
        xvfree(action);
}
```

As I understand it, the dangling pointer problem would only occur if 
someone started statically allocating guest interrupts, right? At least 
for now, all guest interrupts are heap-allocated by route_irq_to_guest().
So my understanding is that we want to ensure that nobody starts 
statically allocating guest interrupts in the future. If that's the 
case, wouldn't it make more sense to add the ASSERT() to 
release_guest_irq(), somewhere before xvfree(info); rather than to 
release_irq()?
~ Oleksii