Re: [PATCH 2/2] iommu/virtio: Avoid using the list iterator past the loop in viommu_add_resv_mem()

[Jean-Philippe Brucker]

On Thu, Jun 04, 2026 at 01:18:16PM +0800, Maoyi Xie wrote:
> viommu_add_resv_mem() walks vdev->resv_regions to find the insertion
> point. When every element has a smaller start address, the
> list_for_each_entry() iterator ends up one past the last entry, and
> &next->list then aliases the list head, so the following list_add_tail()
> still appends at the tail. The result is correct, but using the iterator
> after the loop is undefined per the list_for_each_entry() contract.

Thank you for removing this hack, though I don't find a contract in the
list_for_each_entry() doc, and the fix still accesses a cursor outside the
loop. Since you mentioned C11 UB in another email, do you have more info
on the precise operation which is undefined in the kernel (container_of
into an invalid object or the &next->list addition)?  Just so I can avoid
it in the future.

Anyway, thanks for the patch. If this is just general cleanup that's fine
too.

Reviewed-by: Jean-Philippe Brucker <j...@kernel.org>

> 
> The loop only needs a list_head as the insertion point, so iterate with
> list_for_each() and keep the typed list_entry() dereference inside the loop
> body. No functional change.
> 
> Suggested-by: Robin Murphy <robin.mur...@arm.com>
> Signed-off-by: Maoyi Xie <maoyixie....@gmail.com>

> ---
>  drivers/iommu/virtio-iommu.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 587fc13197f1..1d58d6b626a5 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -486,7 +486,8 @@ static int viommu_add_resv_mem(struct viommu_endpoint 
> *vdev,
>       size_t size;
>       u64 start64, end64;
>       phys_addr_t start, end;
> -     struct iommu_resv_region *region = NULL, *next;
> +     struct iommu_resv_region *region = NULL;
> +     struct list_head *pos;
>       unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>  
>       start = start64 = le64_to_cpu(mem->start);
> @@ -520,11 +521,14 @@ static int viommu_add_resv_mem(struct viommu_endpoint 
> *vdev,
>               return -ENOMEM;
>  
>       /* Keep the list sorted */
> -     list_for_each_entry(next, &vdev->resv_regions, list) {
> +     list_for_each(pos, &vdev->resv_regions) {
> +             struct iommu_resv_region *next =
> +                     list_entry(pos, struct iommu_resv_region, list);
> +
>               if (next->start > region->start)
>                       break;
>       }
> -     list_add_tail(&region->list, &next->list);
> +     list_add_tail(&region->list, pos);
>       return 0;
>  }
>  
> -- 
> 2.34.1
>