iommu: iterator used after loop end in resv region insertion?

iommu: iterator used after loop end in resv region insertion?

[Maoyi Xie]

Hi all,

While reading drivers/iommu/ I noticed two places that look
like a past the end iterator pattern. I would appreciate it
if you could take a look and let me know whether these are
real issues and whether they are worth fixing.

The first is iommu_insert_resv_region() in drivers/iommu/iommu.c
(linux-7.1-rc1, around line 873):

    list_for_each_entry(iter, regions, list) {
            if (nr->start < iter->start ||
                (nr->start == iter->start && nr->type <= iter->type))
                    break;
    }
    list_add_tail(&nr->list, &iter->list);

The second is viommu_add_resv_mem() in drivers/iommu/virtio-iommu.c
(linux-7.1-rc1, around line 523):

    list_for_each_entry(next, &vdev->resv_regions, list) {
            if (next->start > region->start)
                    break;
    }
    list_add_tail(&region->list, &next->list);

In both cases, when the loop walks all entries without break,
the iterator has gone one step past the last entry. &iter->list
then aliases the list head via container_of offset cancellation,
so the insert lands at the list tail. That is the intended
behaviour, but the access is 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). These two sites
in drivers/iommu/ were not covered.

A candidate fix would track an explicit insert_before pointer
initialised to the list head and overwritten to &iter->list
only when the loop breaks early. The 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.

Thanks,
Maoyi Xie
https://maoyixie.com/

Re: iommu: iterator used after loop end in resv region insertion?

[Will Deacon]

On Tue, May 19, 2026 at 02:49:58AM +0800, Maoyi Xie wrote:
> Hi all,
> 
> While reading drivers/iommu/ I noticed two places that look
> like a past the end iterator pattern. I would appreciate it
> if you could take a look and let me know whether these are
> real issues and whether they are worth fixing.
> 
> The first is iommu_insert_resv_region() in drivers/iommu/iommu.c
> (linux-7.1-rc1, around line 873):
> 
>     list_for_each_entry(iter, regions, list) {
>             if (nr->start < iter->start ||
>                 (nr->start == iter->start && nr->type <= iter->type))
>                     break;
>     }
>     list_add_tail(&nr->list, &iter->list);
> 
> The second is viommu_add_resv_mem() in drivers/iommu/virtio-iommu.c
> (linux-7.1-rc1, around line 523):
> 
>     list_for_each_entry(next, &vdev->resv_regions, list) {
>             if (next->start > region->start)
>                     break;
>     }
>     list_add_tail(&region->list, &next->list);
> 
> In both cases, when the loop walks all entries without break,
> the iterator has gone one step past the last entry. &iter->list
> then aliases the list head via container_of offset cancellation,
> so the insert lands at the list tail. That is the intended
> behaviour, but the access is 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). These two sites
> in drivers/iommu/ were not covered.
> 
> A candidate fix would track an explicit insert_before pointer
> initialised to the list head and overwritten to &iter->list
> only when the loop breaks early. The 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.

Given that there's some precedent for "fixing" these type of things,
it's probably best if you just send some patches for these new instances
if you don't mind.

Cheers,

Will

Re: iommu: iterator used after loop end in resv region insertion?

[Robin Murphy]

On 2026-06-02 11:28 am, Will Deacon wrote:
> On Tue, May 19, 2026 at 02:49:58AM +0800, Maoyi Xie wrote:
>> Hi all,
>>
>> While reading drivers/iommu/ I noticed two places that look
>> like a past the end iterator pattern. I would appreciate it
>> if you could take a look and let me know whether these are
>> real issues and whether they are worth fixing.
>>
>> The first is iommu_insert_resv_region() in drivers/iommu/iommu.c
>> (linux-7.1-rc1, around line 873):
>>
>>      list_for_each_entry(iter, regions, list) {
>>              if (nr->start < iter->start ||
>>                  (nr->start == iter->start && nr->type <= iter->type))
>>                      break;
>>      }
>>      list_add_tail(&nr->list, &iter->list);
>>
>> The second is viommu_add_resv_mem() in drivers/iommu/virtio-iommu.c
>> (linux-7.1-rc1, around line 523):
>>
>>      list_for_each_entry(next, &vdev->resv_regions, list) {
>>              if (next->start > region->start)
>>                      break;
>>      }
>>      list_add_tail(&region->list, &next->list);
>>
>> In both cases, when the loop walks all entries without break,
>> the iterator has gone one step past the last entry. &iter->list
>> then aliases the list head via container_of offset cancellation,
>> so the insert lands at the list tail. That is the intended
>> behaviour, but the access is 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). These two sites
>> in drivers/iommu/ were not covered.
>>
>> A candidate fix would track an explicit insert_before pointer
>> initialised to the list head and overwritten to &iter->list
>> only when the loop breaks early. The 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.
> 
> Given that there's some precedent for "fixing" these type of things,
> it's probably best if you just send some patches for these new instances
> if you don't mind.

Agreed, given that the intent in both cases is only to find a list_head 
to use as an insertion point, rather than operate on the entries per se, 
for clarity I'd suggest changing the whole iteration to list_for_each(), 
and keeping the list_entry() dereference strictly inside the loop body.

Thanks,
Robin.

[PATCH 0/2] iommu: avoid using list iterators past the loop in resv region insertion

[Maoyi Xie]

This follows up the inquiry at [1], where Will and Robin agreed that the
two list iterator sites in drivers/iommu/ that run past the loop end are
worth fixing.

Both loops only walk a list to find a list_head to insert before, not to
operate on the entries. Per Robin's suggestion, rather than tracking an
explicit insert_before pointer, each loop now iterates with list_for_each()
and keeps the list_entry() dereference inside the loop body. There is no
functional change in either patch.

[1] https://lore.kernel.org/all/CAHPEe=G-FZvEXjkE+vAXN5MHXCtsOaUoKwg2RQL6m=om+c20hQ@mail.gmail.com/

Maoyi Xie (2):
  iommu: Avoid using the list iterator past the loop in
    iommu_insert_resv_region()
  iommu/virtio: Avoid using the list iterator past the loop in
    viommu_add_resv_mem()

 drivers/iommu/iommu.c        |  7 +++++--
 drivers/iommu/virtio-iommu.c | 10 +++++++---
 2 files changed, 12 insertions(+), 5 deletions(-)


base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
--
2.34.1

[PATCH 1/2] iommu: Avoid using the list iterator past the loop in iommu_insert_resv_region()

[Maoyi Xie]

iommu_insert_resv_region() walks @regions to find the insertion point.
When no element compares greater, the list_for_each_entry() iterator ends
up one past the last entry, so &iter->list aliases the list head through
container_of() offset cancellation and the following list_add_tail() still
targets the tail. The result is correct, but using the iterator after the
loop is undefined per the list_for_each_entry() contract.

The loop only needs a list_head to use as the insertion point, not the
entry itself, 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.murphy@arm.com>
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
 drivers/iommu/iommu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 61c12ba78206..f9f53db9696f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -862,6 +862,7 @@ static int iommu_insert_resv_region(struct iommu_resv_region *new,
 				    struct list_head *regions)
 {
 	struct iommu_resv_region *iter, *tmp, *nr, *top;
+	struct list_head *pos;
 	LIST_HEAD(stack);
 
 	nr = iommu_alloc_resv_region(new->start, new->length,
@@ -870,12 +871,14 @@ static int iommu_insert_resv_region(struct iommu_resv_region *new,
 		return -ENOMEM;
 
 	/* First add the new element based on start address sorting */
-	list_for_each_entry(iter, regions, list) {
+	list_for_each(pos, regions) {
+		iter = list_entry(pos, struct iommu_resv_region, list);
+
 		if (nr->start < iter->start ||
 		    (nr->start == iter->start && nr->type <= iter->type))
 			break;
 	}
-	list_add_tail(&nr->list, &iter->list);
+	list_add_tail(&nr->list, pos);
 
 	/* Merge overlapping segments of type nr->type in @regions, if any */
 	list_for_each_entry_safe(iter, tmp, regions, list) {
-- 
2.34.1

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

[Maoyi Xie]

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.

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.murphy@arm.com>
Signed-off-by: Maoyi Xie <maoyixie.tju@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

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 <jpb@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.murphy@arm.com>
> Signed-off-by: Maoyi Xie <maoyixie.tju@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
> 

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

[Maoyi Xie]

Hi Jean-Philippe,

On Fri, Jun 5, 2026 at 12:04 AM Jean-Philippe Brucker <jpb@kernel.org> wrote:
> 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.

Thanks for the review. You're right.

There is no such contract in the docs. I overstated it. And no undefined
pointer arithmetic happens here either.

iommu_resv_region has list as its first member, so offsetof is 0. That
makes container_of(&vdev->resv_regions, struct iommu_resv_region, list)
just (char *)&vdev->resv_regions - 0, i.e. the head with a different
type. &next->list is the head again. Nothing reads next as an
iommu_resv_region, so no pointer leaves its object.

The container_of would be undefined only if list was not the first
member. Then the subtraction lands before the real object (C11 6.5.6).
That is not the case here.

So this is cleanup, not a UB fix. The typed cursor points at the head but
reads like an entry. That becomes a real bug the day someone reorders the
struct or touches another field. The new pos stays a struct list_head *,
which is just the head, so it avoids all of that. I should have said that
in the message.

If you or Joerg prefer, I can resend the series with the wording fixed.

Thanks,
Maoyi

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

[Jean-Philippe Brucker]

On Fri, Jun 05, 2026 at 04:59:47PM +0800, Maoyi Xie wrote:
> Hi Jean-Philippe,
> 
> On Fri, Jun 5, 2026 at 12:04 AM Jean-Philippe Brucker <jpb@kernel.org> wrote:
> > 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.
> 
> Thanks for the review. You're right.
> 
> There is no such contract in the docs. I overstated it. And no undefined
> pointer arithmetic happens here either.
> 
> iommu_resv_region has list as its first member, so offsetof is 0. That
> makes container_of(&vdev->resv_regions, struct iommu_resv_region, list)
> just (char *)&vdev->resv_regions - 0, i.e. the head with a different
> type. &next->list is the head again. Nothing reads next as an
> iommu_resv_region, so no pointer leaves its object.
> 
> The container_of would be undefined only if list was not the first
> member. Then the subtraction lands before the real object (C11 6.5.6).
> That is not the case here.
> 
> So this is cleanup, not a UB fix. The typed cursor points at the head but
> reads like an entry. That becomes a real bug the day someone reorders the
> struct or touches another field. The new pos stays a struct list_head *,
> which is just the head, so it avoids all of that. I should have said that
> in the message.
> 
> If you or Joerg prefer, I can resend the series with the wording fixed.

For me the patch is fine as is, I was just curious about what the kernel
expects for pointer arithmetic. Thanks for clarifying

Thanks,
Jean

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

[Robin Murphy]

On 2026-06-05 9:59 am, Maoyi Xie wrote:
> Hi Jean-Philippe,
> 
> On Fri, Jun 5, 2026 at 12:04 AM Jean-Philippe Brucker <jpb@kernel.org> wrote:
>> 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.
> 
> Thanks for the review. You're right.
> 
> There is no such contract in the docs. I overstated it. And no undefined
> pointer arithmetic happens here either.
> 
> iommu_resv_region has list as its first member, so offsetof is 0. That
> makes container_of(&vdev->resv_regions, struct iommu_resv_region, list)
> just (char *)&vdev->resv_regions - 0, i.e. the head with a different
> type. &next->list is the head again. Nothing reads next as an
> iommu_resv_region, so no pointer leaves its object.
> 
> The container_of would be undefined only if list was not the first
> member. Then the subtraction lands before the real object (C11 6.5.6).
> That is not the case here.

If that were a real issue, then I think list_entry_is_head() in the 
list_for_each_entry() iteration itself would suffer from it too. It's 
probably safe to say that while the C language in general leaves this 
open, Linux relies on GCC using pointer representations where the 
arithmetic will always work out, same as we depend on two's complement 
representation of integers all over the place.

I believe the concern is more just that if the pattern of using a 
list_entry iterator variable outside the scope of the loop isn't 
discouraged, then it's all too easy for subsequent code to inadvertently 
dereference fields *other* than the list_head without checking that it 
is a valid entry, which definitely would then be the worst kind of UB.

Thanks,
Robin.

> So this is cleanup, not a UB fix. The typed cursor points at the head but
> reads like an entry. That becomes a real bug the day someone reorders the
> struct or touches another field. The new pos stays a struct list_head *,
> which is just the head, so it avoids all of that. I should have said that
> in the message.
> 
> If you or Joerg prefer, I can resend the series with the wording fixed.
> 
> Thanks,
> Maoyi