Re: [PATCH v2] x86/apic: Avoid infinite loop in io_apic_level_ack_pending()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Oct 15, 2025 at 01:14:20PM -0400, Jason Andryuk wrote:
> On 2025-10-15 08:59, Jan Beulich wrote:
> > On 14.10.2025 09:37, Roger Pau Monné wrote:
> > > On Mon, Oct 13, 2025 at 05:11:06PM -0400, Jason Andryuk wrote:
> > > > io_apic_level_ack_pending() will end up in an infinite loop if
> > > > entry->pin == -1.  entry does not change, so it will keep reading -1.
> > > 
> > > Do you know how you end up with an entry with pin == -1 on the
> > > irq_pin_list? Are there systems with gaps in the GSI space between
> > > IO-APICs?  So far everything I saw had the IO-APIC in contiguous GSI
> > > space.
> > > 
> > > > Convert to a proper for loop so that continue works.  Add a new helper,
> > > > next_entry(), to handle advancing to the next irq_pin_list entry.
> > > > 
> > > > Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
> > > > Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> > > > ---
> > > > v2:
> > > > continue (not break) for pin == -1.
> > > > 
> > > > I added the next_entry() helper since putting the expression in the for
> > > > loop is a little cluttered.  The helper can also be re-used for other
> > > > instances within the file.
> > 
> > Would this intention ...
> > 
> > > > --- a/xen/arch/x86/io_apic.c
> > > > +++ b/xen/arch/x86/io_apic.c
> > > > @@ -1586,14 +1586,21 @@ static int __init cf_check 
> > > > setup_ioapic_ack(const char *s)
> > > >   }
> > > >   custom_param("ioapic_ack", setup_ioapic_ack);
> > > > +static struct irq_pin_list *next_entry(struct irq_pin_list *entry)
> > > 
> > > I think you can make the entry parameter const?
> > 
> > ... possibly conflict with such a change?
> 
> I changed only the parameter to const, and the return value is still
> non-const.  So I think that will be re-usable.
> 
> I placed next_entry() immediately before its use in
> io_apic_level_ack_pending().  It would need to be earlier in the file to be
> used more.  Should I move its addition earlier?
> 
> And another Minor question.  Roger asked for ~Linux style in the for loop.
> But in next_entry() I have Xen style:
>     if ( !entry->next )
> 
> Should I switch to:
>     if (!entry->next)
> 
> ?

IMO for complete functions newly introduced it's fine to use Xen
style, I don't think we will ever import anything else from Linux to
this file, we have already diverged too much.

Regards, Roger.