[PATCH rc 0/2] Fix two bugs in iommu gather processing

[PATCH rc 0/2] Fix two bugs in iommu gather processing

[Jason Gunthorpe]

I've been working on an invalidation kunit test and found two issues worth
fixing right away.

Jason Gunthorpe (2):
  iommu: Do not call drivers for empty gathers
  iommupt: Fix short gather if the unmap goes into a large mapping

 drivers/iommu/generic_pt/iommu_pt.h | 2 +-
 include/linux/iommu.h               | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)


base-commit: 10595d49f542df65ad4107713017075d5b9b529c
-- 
2.43.0

[PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Jason Gunthorpe]

An empty gather is coded with start=U64_MAX, end=0 and several drivers go
on to convert that to a size with:

 end - start + 1

Which gives 2 for an empty gather. This then causes Weird Stuff to
happen (for example an UBSAN splat in VT-d) that is hopefully harmless,
but maybe not.

Prevent drivers from being called right in iommu_iotlb_sync().

Auditing shows that AMD, Intel, Mediatek and RSIC-V drivers all do things
on these empty gathers.

Further, there are several callers that can trigger empty gathers,
especially in unusual conditions. For example iommu_map_nosync() will call
a 0 size unmap on some error paths. Also in VFIO, iommupt and other
places.

Cc: stable@vger.kernel.org
Reported-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Closes: https://lore.kernel.org/r/11145826.aFP6jjVeTY@jkrzyszt-mobl2.ger.corp.intel.com
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 include/linux/iommu.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 54b8b48c762e88..555597b54083cd 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -980,7 +980,8 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain)
 static inline void iommu_iotlb_sync(struct iommu_domain *domain,
 				  struct iommu_iotlb_gather *iotlb_gather)
 {
-	if (domain->ops->iotlb_sync)
+	if (domain->ops->iotlb_sync &&
+	    likely(iotlb_gather->start < iotlb_gather->end))
 		domain->ops->iotlb_sync(domain, iotlb_gather);
 
 	iommu_iotlb_gather_init(iotlb_gather);
-- 
2.43.0

[PATCH rc 2/2] iommupt: Fix short gather if the unmap goes into a large mapping

[Jason Gunthorpe]

unmap has the odd behavior that it can unmap more than requested if the
ending point lands within the middle of a large or contiguous IOPTE.

In this case the gather should flush everything unmapped which can be
larger than what was requested to be unmapped. The gather was only
flushing the range requested to be unmapped, not extending to the extra
range, resulting in a short invalidation if the caller hits this special
condition.

This was found by the new invalidation/gather test I am adding in
preparation for ARMv8. Claude deduced the root cause.

As far as I remember nothing relies on unmapping a large entry, so this is
likely not a triggerable bug.

Cc: stable@vger.kernel.org
Fixes: 7c53f4238aa8 ("iommupt: Add unmap_pages op")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/generic_pt/iommu_pt.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/generic_pt/iommu_pt.h b/drivers/iommu/generic_pt/iommu_pt.h
index 3e33fe64feab22..7e7a6e7abdeed1 100644
--- a/drivers/iommu/generic_pt/iommu_pt.h
+++ b/drivers/iommu/generic_pt/iommu_pt.h
@@ -1057,7 +1057,7 @@ size_t DOMAIN_NS(unmap_pages)(struct iommu_domain *domain, unsigned long iova,
 
 	pt_walk_range(&range, __unmap_range, &unmap);
 
-	gather_range_pages(iotlb_gather, iommu_table, iova, len,
+	gather_range_pages(iotlb_gather, iommu_table, iova, unmap.unmapped,
 			   &unmap.free_list);
 
 	return unmap.unmapped;
-- 
2.43.0

Re: [PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Vasant Hegde]

On 3/3/2026 3:52 AM, Jason Gunthorpe wrote:
> An empty gather is coded with start=U64_MAX, end=0 and several drivers go
> on to convert that to a size with:
> 
>  end - start + 1
> 
> Which gives 2 for an empty gather. This then causes Weird Stuff to
> happen (for example an UBSAN splat in VT-d) that is hopefully harmless,
> but maybe not.
> 
> Prevent drivers from being called right in iommu_iotlb_sync().
> 
> Auditing shows that AMD, Intel, Mediatek and RSIC-V drivers all do things
> on these empty gathers.
> 
> Further, there are several callers that can trigger empty gathers,
> especially in unusual conditions. For example iommu_map_nosync() will call
> a 0 size unmap on some error paths. Also in VFIO, iommupt and other
> places.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Closes: https://lore.kernel.org/r/11145826.aFP6jjVeTY@jkrzyszt-mobl2.ger.corp.intel.com
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH rc 2/2] iommupt: Fix short gather if the unmap goes into a large mapping

[Vasant Hegde]

On 3/3/2026 3:52 AM, Jason Gunthorpe wrote:
> unmap has the odd behavior that it can unmap more than requested if the
> ending point lands within the middle of a large or contiguous IOPTE.
> 
> In this case the gather should flush everything unmapped which can be
> larger than what was requested to be unmapped. The gather was only
> flushing the range requested to be unmapped, not extending to the extra
> range, resulting in a short invalidation if the caller hits this special
> condition.
> 
> This was found by the new invalidation/gather test I am adding in
> preparation for ARMv8. Claude deduced the root cause.
> 
> As far as I remember nothing relies on unmapping a large entry, so this is
> likely not a triggerable bug.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7c53f4238aa8 ("iommupt: Add unmap_pages op")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>


Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Robin Murphy]

On 2026-03-02 10:22 pm, Jason Gunthorpe wrote:
> An empty gather is coded with start=U64_MAX, end=0 and several drivers go
> on to convert that to a size with:
> 
>   end - start + 1
> 
> Which gives 2 for an empty gather. This then causes Weird Stuff to
> happen (for example an UBSAN splat in VT-d) that is hopefully harmless,
> but maybe not.
> 
> Prevent drivers from being called right in iommu_iotlb_sync().
> 
> Auditing shows that AMD, Intel, Mediatek and RSIC-V drivers all do things
> on these empty gathers.
> 
> Further, there are several callers that can trigger empty gathers,
> especially in unusual conditions. For example iommu_map_nosync() will call
> a 0 size unmap on some error paths. Also in VFIO, iommupt and other
> places.

My instinct is still to tidy up the 0-length unmap case(s), but I guess 
iommu_iotlb_sync() is itself also a public API where being more robust 
against erroneous usage is no bad thing. With one minor nit below,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Cc: stable@vger.kernel.org
> Reported-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Closes: https://lore.kernel.org/r/11145826.aFP6jjVeTY@jkrzyszt-mobl2.ger.corp.intel.com
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   include/linux/iommu.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 54b8b48c762e88..555597b54083cd 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -980,7 +980,8 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain)
>   static inline void iommu_iotlb_sync(struct iommu_domain *domain,
>   				  struct iommu_iotlb_gather *iotlb_gather)
>   {
> -	if (domain->ops->iotlb_sync)
> +	if (domain->ops->iotlb_sync &&
> +	    likely(iotlb_gather->start < iotlb_gather->end))

Elsewhere we just use "gather->end != 0" as the "non-empty" condition; 
how concerned are we about defending against more-intentionally 
malformed gathers here?

Thanks,
Robin.

>   		domain->ops->iotlb_sync(domain, iotlb_gather);
>   
>   	iommu_iotlb_gather_init(iotlb_gather);

Re: [PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Jason Gunthorpe]

On Tue, Mar 03, 2026 at 12:53:28PM +0000, Robin Murphy wrote:

> > Further, there are several callers that can trigger empty gathers,
> > especially in unusual conditions. For example iommu_map_nosync() will call
> > a 0 size unmap on some error paths. Also in VFIO, iommupt and other
> > places.
> 
> My instinct is still to tidy up the 0-length unmap case(s), but I guess
> iommu_iotlb_sync() is itself also a public API where being more robust
> against erroneous usage is no bad thing. 

I also wanted to do that but found enough problematic cases I lost
confidence I could reliably catch them all..

> > -	if (domain->ops->iotlb_sync)
> > +	if (domain->ops->iotlb_sync &&
> > +	    likely(iotlb_gather->start < iotlb_gather->end))
> 
> Elsewhere we just use "gather->end != 0" as the "non-empty" condition; how
> concerned are we about defending against more-intentionally malformed
> gathers here?

I choose this deliberately to protect the driver, a malformed gather
that is 0 sized, or negative sized looks like it will have Weird
Things happen in drivers.

We could further classify the < and WARN_ON the malformed cases, but I
don't want to pass negative sized gathers into drivers. We'd probably
also have to de-inline the function if more is added. Do you have a
preference?

Jason

Re: [PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Robin Murphy]

On 2026-03-03 1:04 pm, Jason Gunthorpe wrote:
> On Tue, Mar 03, 2026 at 12:53:28PM +0000, Robin Murphy wrote:
> 
>>> Further, there are several callers that can trigger empty gathers,
>>> especially in unusual conditions. For example iommu_map_nosync() will call
>>> a 0 size unmap on some error paths. Also in VFIO, iommupt and other
>>> places.
>>
>> My instinct is still to tidy up the 0-length unmap case(s), but I guess
>> iommu_iotlb_sync() is itself also a public API where being more robust
>> against erroneous usage is no bad thing.
> 
> I also wanted to do that but found enough problematic cases I lost
> confidence I could reliably catch them all..

I reckon an early "if (!size) return 0;" in iommu_unmap() would suffice 
to cover the internal error cleanup paths and most careless external 
users. However if we don't trust iommu_unmap_fast() users to always do 
the right thing either then we want this check in iommu_iotlb_sync() 
anyway, at which point the iommu_unmap() check would really only serve 
to skip a bit more unnecessary work on error cleanup paths, and do we 
really care about optimising errors? Hence I'm satisfied that this patch 
does in fact seem to be the best option.

>>> -	if (domain->ops->iotlb_sync)
>>> +	if (domain->ops->iotlb_sync &&
>>> +	    likely(iotlb_gather->start < iotlb_gather->end))
>>
>> Elsewhere we just use "gather->end != 0" as the "non-empty" condition; how
>> concerned are we about defending against more-intentionally malformed
>> gathers here?
> 
> I choose this deliberately to protect the driver, a malformed gather
> that is 0 sized, or negative sized looks like it will have Weird
> Things happen in drivers.
> 
> We could further classify the < and WARN_ON the malformed cases, but I
> don't want to pass negative sized gathers into drivers. We'd probably
> also have to de-inline the function if more is added. Do you have a
> preference?

No, that's fine, I just wanted to confirm the intent - this isn't a 
place where we should need to be concerned about micro-optimising to 
maybe save a load and an extra ALU op or two, just that I don't think 
it's worth doing any more than strictly necessary for our own 
robustness. Thus there's no need to change the check in 
iommu_iotlb_gather_is_disjoint() either, as that now just serves to skip 
the redundant reinitialisation of an already-empty gather, which is 
justifiably a different thing from the actual validity-of-sync condition 
anyway.

Cheers,
Robin.

Re: [PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Samiullah Khawaja]

On Mon, Mar 02, 2026 at 06:22:52PM -0400, Jason Gunthorpe wrote:
>An empty gather is coded with start=U64_MAX, end=0 and several drivers go
>on to convert that to a size with:
>
> end - start + 1
>
>Which gives 2 for an empty gather. This then causes Weird Stuff to
>happen (for example an UBSAN splat in VT-d) that is hopefully harmless,
>but maybe not.
>
>Prevent drivers from being called right in iommu_iotlb_sync().
>
>Auditing shows that AMD, Intel, Mediatek and RSIC-V drivers all do things
>on these empty gathers.
>
>Further, there are several callers that can trigger empty gathers,
>especially in unusual conditions. For example iommu_map_nosync() will call
>a 0 size unmap on some error paths. Also in VFIO, iommupt and other
>places.
>
>Cc: stable@vger.kernel.org
>Reported-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Closes: https://lore.kernel.org/r/11145826.aFP6jjVeTY@jkrzyszt-mobl2.ger.corp.intel.com
>Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>---
> include/linux/iommu.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>index 54b8b48c762e88..555597b54083cd 100644
>--- a/include/linux/iommu.h
>+++ b/include/linux/iommu.h
>@@ -980,7 +980,8 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain)
> static inline void iommu_iotlb_sync(struct iommu_domain *domain,
> 				  struct iommu_iotlb_gather *iotlb_gather)
> {
>-	if (domain->ops->iotlb_sync)
>+	if (domain->ops->iotlb_sync &&
>+	    likely(iotlb_gather->start < iotlb_gather->end))
> 		domain->ops->iotlb_sync(domain, iotlb_gather);
>
> 	iommu_iotlb_gather_init(iotlb_gather);
>-- 
>2.43.0
>

Reviewed-by: Samiullah Khawaja <skhawaja@google.com>

Re: [PATCH rc 2/2] iommupt: Fix short gather if the unmap goes into a large mapping

[Samiullah Khawaja]

On Mon, Mar 02, 2026 at 06:22:53PM -0400, Jason Gunthorpe wrote:
>unmap has the odd behavior that it can unmap more than requested if the
>ending point lands within the middle of a large or contiguous IOPTE.
>
>In this case the gather should flush everything unmapped which can be
>larger than what was requested to be unmapped. The gather was only
>flushing the range requested to be unmapped, not extending to the extra
>range, resulting in a short invalidation if the caller hits this special
>condition.
>
>This was found by the new invalidation/gather test I am adding in
>preparation for ARMv8. Claude deduced the root cause.
>
>As far as I remember nothing relies on unmapping a large entry, so this is
>likely not a triggerable bug.
>
>Cc: stable@vger.kernel.org
>Fixes: 7c53f4238aa8 ("iommupt: Add unmap_pages op")
>Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>---
> drivers/iommu/generic_pt/iommu_pt.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/iommu/generic_pt/iommu_pt.h b/drivers/iommu/generic_pt/iommu_pt.h
>index 3e33fe64feab22..7e7a6e7abdeed1 100644
>--- a/drivers/iommu/generic_pt/iommu_pt.h
>+++ b/drivers/iommu/generic_pt/iommu_pt.h
>@@ -1057,7 +1057,7 @@ size_t DOMAIN_NS(unmap_pages)(struct iommu_domain *domain, unsigned long iova,
>
> 	pt_walk_range(&range, __unmap_range, &unmap);
>
>-	gather_range_pages(iotlb_gather, iommu_table, iova, len,
>+	gather_range_pages(iotlb_gather, iommu_table, iova, unmap.unmapped,
> 			   &unmap.free_list);
>
> 	return unmap.unmapped;
>-- 
>2.43.0
>

Reviewed-by: Samiullah Khawaja <skhawaja@google.com>

Re: [PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Baolu Lu]

On 3/3/26 06:22, Jason Gunthorpe wrote:
> An empty gather is coded with start=U64_MAX, end=0 and several drivers go
> on to convert that to a size with:
> 
>   end - start + 1
> 
> Which gives 2 for an empty gather. This then causes Weird Stuff to
> happen (for example an UBSAN splat in VT-d) that is hopefully harmless,
> but maybe not.
> 
> Prevent drivers from being called right in iommu_iotlb_sync().
> 
> Auditing shows that AMD, Intel, Mediatek and RSIC-V drivers all do things
> on these empty gathers.
> 
> Further, there are several callers that can trigger empty gathers,
> especially in unusual conditions. For example iommu_map_nosync() will call
> a 0 size unmap on some error paths. Also in VFIO, iommupt and other
> places.
> 
> Cc:stable@vger.kernel.org
> Reported-by: Janusz Krzysztofik<janusz.krzysztofik@linux.intel.com>
> Closes:https://lore.kernel.org/r/11145826.aFP6jjVeTY@jkrzyszt- 
> mobl2.ger.corp.intel.com
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Re: [PATCH rc 2/2] iommupt: Fix short gather if the unmap goes into a large mapping

[Baolu Lu]

On 3/3/26 06:22, Jason Gunthorpe wrote:
> unmap has the odd behavior that it can unmap more than requested if the
> ending point lands within the middle of a large or contiguous IOPTE.
> 
> In this case the gather should flush everything unmapped which can be
> larger than what was requested to be unmapped. The gather was only
> flushing the range requested to be unmapped, not extending to the extra
> range, resulting in a short invalidation if the caller hits this special
> condition.
> 
> This was found by the new invalidation/gather test I am adding in
> preparation for ARMv8. Claude deduced the root cause.
> 
> As far as I remember nothing relies on unmapping a large entry, so this is
> likely not a triggerable bug.
> 
> Cc:stable@vger.kernel.org
> Fixes: 7c53f4238aa8 ("iommupt: Add unmap_pages op")
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Re: [PATCH rc 1/2] iommu: Do not call drivers for empty gathers

[Janusz Krzysztofik]

Hi,

On Tue, 2026-03-03 at 15:56 +0000, Robin Murphy wrote:
> On 2026-03-03 1:04 pm, Jason Gunthorpe wrote:
> > On Tue, Mar 03, 2026 at 12:53:28PM +0000, Robin Murphy wrote:
> > 
> > > > Further, there are several callers that can trigger empty gathers,
> > > > especially in unusual conditions. For example iommu_map_nosync() will call
> > > > a 0 size unmap on some error paths. Also in VFIO, iommupt and other
> > > > places.
> > > 
> > > My instinct is still to tidy up the 0-length unmap case(s), but I guess
> > > iommu_iotlb_sync() is itself also a public API where being more robust
> > > against erroneous usage is no bad thing.
> > 
> > I also wanted to do that but found enough problematic cases I lost
> > confidence I could reliably catch them all..
> 
> I reckon an early "if (!size) return 0;" in iommu_unmap() would suffice 
> to cover the internal error cleanup paths and most careless external 
> users. However if we don't trust iommu_unmap_fast() users to always do 
> the right thing either then we want this check in iommu_iotlb_sync() 
> anyway, at which point the iommu_unmap() check would really only serve 
> to skip a bit more unnecessary work on error cleanup paths, and do we 
> really care about optimising errors? Hence I'm satisfied that this patch 
> does in fact seem to be the best option.

Is this patch going to be applied soon to one of your branches? I'm 
waiting for that to happen because I want to cherry-pick it and propose as 
a temporary fix to be applied to our core-for-CI sub-branch of drm-tip 
until the original lands in mainline.

Thanks,
Janusz

> 
> > > > -	if (domain->ops->iotlb_sync)
> > > > +	if (domain->ops->iotlb_sync &&
> > > > +	    likely(iotlb_gather->start < iotlb_gather->end))
> > > 
> > > Elsewhere we just use "gather->end != 0" as the "non-empty" condition; how
> > > concerned are we about defending against more-intentionally malformed
> > > gathers here?
> > 
> > I choose this deliberately to protect the driver, a malformed gather
> > that is 0 sized, or negative sized looks like it will have Weird
> > Things happen in drivers.
> > 
> > We could further classify the < and WARN_ON the malformed cases, but I
> > don't want to pass negative sized gathers into drivers. We'd probably
> > also have to de-inline the function if more is added. Do you have a
> > preference?
> 
> No, that's fine, I just wanted to confirm the intent - this isn't a 
> place where we should need to be concerned about micro-optimising to 
> maybe save a load and an extra ALU op or two, just that I don't think 
> it's worth doing any more than strictly necessary for our own 
> robustness. Thus there's no need to change the check in 
> iommu_iotlb_gather_is_disjoint() either, as that now just serves to skip 
> the redundant reinitialisation of an already-empty gather, which is 
> justifiably a different thing from the actual validity-of-sync condition 
> anyway.
> 
> Cheers,
> Robin.