Re: acpi/riscv: iterator after loop end in riscv_acpi_register_ext_intc?

[Sunil V L]

Hi Maoyi Xie,

On Tue, May 19, 2026 at 2:40 PM Maoyi Xie <maoyixie.tju@xxxxxxxxx> wrote:
>
> Hi all,
>
> While reading drivers/acpi/riscv/irq.c I noticed something
> that looks like a past the end iterator pattern, used in two
> places in the same function. I would appreciate it if you
> could take a look and let me know whether this is a real
> issue, and whether it is worth fixing.
>
> The site is riscv_acpi_register_ext_intc() (linux-7.1-rc1,
> around line 155):
>
>     list_for_each_entry(node, &ext_intc_list, list) {
>             if (node->gsi_base < ext_intc_element->gsi_base)
>                     break;
>     }
>
>     /* Adjust the previous node's GSI range if that has
>        pending registration */
>     prev = list_prev_entry(node, list);
>     if (!list_entry_is_head(prev, &ext_intc_list, list)) {
>             if (prev->flag & RISCV_ACPI_INTC_FLAG_PENDING)
>                     prev->nr_irqs = ext_intc_element->gsi_base
>                                     - prev->gsi_base;
>     }
>
>     list_add_tail(&ext_intc_element->list, &node->list);
>
> When the loop walks all entries without break (no entry has
> gsi_base smaller than the new one), node is past the end.
> The function then uses node in two ways:
>
>   1. list_prev_entry(node, list) relies on &node->list
>      aliasing &ext_intc_list (the head) via container_of
>      offset cancellation, so the result becomes the last
>      real entry of the list (head->prev). The PENDING flag
>      adjustment then runs on that last entry.
>
>   2. list_add_tail(..., &node->list) relies on the same
>      alias so the insert lands at the list tail.
>
> Both behaviours are intended (sorted insertion plus a
> fall-through path that still wants the last entry's PENDING
> flag adjusted), but both dereferences of the past the end
> node are undefined per C11.
>
> Jakob Koschel cleaned up many such sites in 2022, for example
> commits 99d8ae4ec8a (tracing: Remove usage of list iterator
> variable after the loop), 2966a9918df (clockevents: Use
> dedicated list iterator variable) and dc1acd5c946 (dlm:
> replace usage of found with dedicated list iterator
> variable). This site was not covered.
>
> A candidate fix would track an explicit insertion target
> (struct list_head *pos = &ext_intc_list) which is overwritten
> to &node->list only when the loop breaks early. The PENDING
> flag adjustment then reads pos->prev directly: if pos->prev
> is &ext_intc_list (empty list, or break on the first entry),
> the adjustment is skipped; otherwise pos->prev points to the
> real entry that becomes the predecessor in list order. In the
> fall-through path pos stays at the list head, and head->prev
> is the last real entry, so that branch keeps adjusting the
> last entry as before. Observable behaviour is unchanged.
>
> If this is intentional or already known, please disregard.
> Otherwise, I am happy to send a [PATCH] or to leave the fix
> to you. Thank you for your time, and sorry for the noise if
> this is not actually worth fixing or has already been spotted.
>
Yes, I think this should be fixed. Thanks for reporting the issue.
Please send the patch.

Thanks,
Sunil