[PATCH v2] Fixes a race in iopt_unmap_iova_range

[PATCH v2] Fixes a race in iopt_unmap_iova_range

[Sina Hassani]

Bug: iopt_unmap_iova_range releases the lock on iova_rwsem inside the loop
body when getting to the more expensive unmap operations. This is fine on
its own except the loop condition is based on the first area that matches
the unmap address range. If a concurrent call to map picks an area that was
unmapped in the previous iterations, this loop will try to mistakenly unmap
them.

How to reproduce: I was able to reproduce this by having one userspace
thread mapping buffers and passing them to another thread that maps
them. The problem easily shows up as ebusy errors if you use single page
mappings.

The fix: A simple fix that I implemented here is to advance the start
pointer after we unmap an area. That way we are only looking at the
IOVA range that is mapped and hence guaranteed to not have any overlaps
in each iteration.

Test: I tested this against the repro mentioned above and it works fine.

Cc: stable@vger.kernel.org
Signed-off-by: Sina Hassani <sina@openai.com>
---
 drivers/iommu/iommufd/io_pagetable.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iommu/iommufd/io_pagetable.c
b/drivers/iommu/iommufd/io_pagetable.c
index ee003bb2f647..c69af341219a 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -814,6 +814,15 @@ static int iopt_unmap_iova_range(struct
io_pagetable *iopt, unsigned long start,
                unmapped_bytes += area_last - area_first + 1;

                down_write(&iopt->iova_rwsem);
+
+               /* Do not reconsider things already unmapped in case of
+                * concurrent allocation */
+               start = area_last + 1;
+               if (start < area_last) {
+                       /* Overflow. IOVA ranges do not wrap around so we can
+                        * exit here */
+                       break;
+               }
        }

 out_unlock_iova:
--
2.43.0

Re: [PATCH v2] Fixes a race in iopt_unmap_iova_range

[Jason Gunthorpe]

On Mon, Apr 06, 2026 at 04:07:01PM -0700, Sina Hassani wrote:

> io_pagetable *iopt, unsigned long start,
>                 unmapped_bytes += area_last - area_first + 1;
> 
>                 down_write(&iopt->iova_rwsem);
> +
> +               /* Do not reconsider things already unmapped in case of
> +                * concurrent allocation */
> +               start = area_last + 1;

area_last can be ULONG_MAX so this literally overflows to 0. It is why
I formed the suggestion I gave as I did

Jason

Re: [PATCH v2] Fixes a race in iopt_unmap_iova_range

[Sina Hassani]

On Mon, Apr 6, 2026 at 6:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Apr 06, 2026 at 04:07:01PM -0700, Sina Hassani wrote:
>
> > io_pagetable *iopt, unsigned long start,
> >                 unmapped_bytes += area_last - area_first + 1;
> >
> >                 down_write(&iopt->iova_rwsem);
> > +
> > +               /* Do not reconsider things already unmapped in case of
> > +                * concurrent allocation */
> > +               start = area_last + 1;
>
> area_last can be ULONG_MAX so this literally overflows to 0. It is why
> I formed the suggestion I gave as I did
>
Yes, in which case the  if (start < area_last) that follows will catch
it. Are you suggesting I compare against ULONG_MAX instead?
> Jason

Re: [PATCH v2] Fixes a race in iopt_unmap_iova_range

[Jason Gunthorpe]

On Mon, Apr 06, 2026 at 06:17:24PM -0700, Sina Hassani wrote:
> On Mon, Apr 6, 2026 at 6:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Mon, Apr 06, 2026 at 04:07:01PM -0700, Sina Hassani wrote:
> >
> > > io_pagetable *iopt, unsigned long start,
> > >                 unmapped_bytes += area_last - area_first + 1;
> > >
> > >                 down_write(&iopt->iova_rwsem);
> > > +
> > > +               /* Do not reconsider things already unmapped in case of
> > > +                * concurrent allocation */
> > > +               start = area_last + 1;
> >
> > area_last can be ULONG_MAX so this literally overflows to 0. It is why
> > I formed the suggestion I gave as I did
> >
> Yes, in which case the  if (start < area_last) that follows will catch
> it. Are you suggesting I compare against ULONG_MAX instead?

iommufd does not have any overflows to 0 and rely on it tricks like
this. You should just compare to the existing iteration last

Jason

Re: [PATCH v2] Fixes a race in iopt_unmap_iova_range

[Sina Hassani]

On Mon, Apr 6, 2026 at 6:27 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Apr 06, 2026 at 06:17:24PM -0700, Sina Hassani wrote:
> > On Mon, Apr 6, 2026 at 6:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > On Mon, Apr 06, 2026 at 04:07:01PM -0700, Sina Hassani wrote:
> > >
> > > > io_pagetable *iopt, unsigned long start,
> > > >                 unmapped_bytes += area_last - area_first + 1;
> > > >
> > > >                 down_write(&iopt->iova_rwsem);
> > > > +
> > > > +               /* Do not reconsider things already unmapped in case of
> > > > +                * concurrent allocation */
> > > > +               start = area_last + 1;
> > >
> > > area_last can be ULONG_MAX so this literally overflows to 0. It is why
> > > I formed the suggestion I gave as I did
> > >
> > Yes, in which case the  if (start < area_last) that follows will catch
> > it. Are you suggesting I compare against ULONG_MAX instead?
>
> iommufd does not have any overflows to 0 and rely on it tricks like
> this. You should just compare to the existing iteration last
>
Just to confirm that I understand correctly, like this?

+               if (area_last >= last) {
+                       break;
+.              } else {
+.                      start = area_last + 1;
+               }

> Jason

Re: [PATCH v2] Fixes a race in iopt_unmap_iova_range

[Sina Hassani]

Friendly ping

On Mon, Apr 6, 2026 at 6:40 PM Sina Hassani <sina@openai.com> wrote:
>
> On Mon, Apr 6, 2026 at 6:27 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Mon, Apr 06, 2026 at 06:17:24PM -0700, Sina Hassani wrote:
> > > On Mon, Apr 6, 2026 at 6:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > >
> > > > On Mon, Apr 06, 2026 at 04:07:01PM -0700, Sina Hassani wrote:
> > > >
> > > > > io_pagetable *iopt, unsigned long start,
> > > > >                 unmapped_bytes += area_last - area_first + 1;
> > > > >
> > > > >                 down_write(&iopt->iova_rwsem);
> > > > > +
> > > > > +               /* Do not reconsider things already unmapped in case of
> > > > > +                * concurrent allocation */
> > > > > +               start = area_last + 1;
> > > >
> > > > area_last can be ULONG_MAX so this literally overflows to 0. It is why
> > > > I formed the suggestion I gave as I did
> > > >
> > > Yes, in which case the  if (start < area_last) that follows will catch
> > > it. Are you suggesting I compare against ULONG_MAX instead?
> >
> > iommufd does not have any overflows to 0 and rely on it tricks like
> > this. You should just compare to the existing iteration last
> >
> Just to confirm that I understand correctly, like this?
>
> +               if (area_last >= last) {
> +                       break;
> +.              } else {
> +.                      start = area_last + 1;
> +               }
>
> > Jason

Re: [PATCH v2] Fixes a race in iopt_unmap_iova_range

[Jason Gunthorpe]

On Mon, Apr 06, 2026 at 06:40:05PM -0700, Sina Hassani wrote:
> On Mon, Apr 6, 2026 at 6:27 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Mon, Apr 06, 2026 at 06:17:24PM -0700, Sina Hassani wrote:
> > > On Mon, Apr 6, 2026 at 6:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > >
> > > > On Mon, Apr 06, 2026 at 04:07:01PM -0700, Sina Hassani wrote:
> > > >
> > > > > io_pagetable *iopt, unsigned long start,
> > > > >                 unmapped_bytes += area_last - area_first + 1;
> > > > >
> > > > >                 down_write(&iopt->iova_rwsem);
> > > > > +
> > > > > +               /* Do not reconsider things already unmapped in case of
> > > > > +                * concurrent allocation */
> > > > > +               start = area_last + 1;
> > > >
> > > > area_last can be ULONG_MAX so this literally overflows to 0. It is why
> > > > I formed the suggestion I gave as I did
> > > >
> > > Yes, in which case the  if (start < area_last) that follows will catch
> > > it. Are you suggesting I compare against ULONG_MAX instead?
> >
> > iommufd does not have any overflows to 0 and rely on it tricks like
> > this. You should just compare to the existing iteration last
> >
> Just to confirm that I understand correctly, like this?
> 
> +               if (area_last >= last) {
> +                       break;
> +.              } else {
> +.                      start = area_last + 1;
> +               }

Yeah that looks Ok

Jason

Re: [PATCH v2] Fixes a race in iopt_unmap_iova_range

[Sina Hassani]

On Thu, Apr 9, 2026 at 8:16 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Apr 06, 2026 at 06:40:05PM -0700, Sina Hassani wrote:
> > On Mon, Apr 6, 2026 at 6:27 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > On Mon, Apr 06, 2026 at 06:17:24PM -0700, Sina Hassani wrote:
> > > > On Mon, Apr 6, 2026 at 6:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > > >
> > > > > On Mon, Apr 06, 2026 at 04:07:01PM -0700, Sina Hassani wrote:
> > > > >
> > > > > > io_pagetable *iopt, unsigned long start,
> > > > > >                 unmapped_bytes += area_last - area_first + 1;
> > > > > >
> > > > > >                 down_write(&iopt->iova_rwsem);
> > > > > > +
> > > > > > +               /* Do not reconsider things already unmapped in case of
> > > > > > +                * concurrent allocation */
> > > > > > +               start = area_last + 1;
> > > > >
> > > > > area_last can be ULONG_MAX so this literally overflows to 0. It is why
> > > > > I formed the suggestion I gave as I did
> > > > >
> > > > Yes, in which case the  if (start < area_last) that follows will catch
> > > > it. Are you suggesting I compare against ULONG_MAX instead?
> > >
> > > iommufd does not have any overflows to 0 and rely on it tricks like
> > > this. You should just compare to the existing iteration last
> > >
> > Just to confirm that I understand correctly, like this?
> >
> > +               if (area_last >= last) {
> > +                       break;
> > +.              } else {
> > +.                      start = area_last + 1;
> > +               }
>
> Yeah that looks Ok
Sounds good, sent you the v3
>
> Jason