The callers of 'iommu_iotlb_gather_queued' always have a non-null
'gather', so we don't need to check for the same in the function.

Signed-off-by: Aashish Sharma (Google) <aashish@aashishsharma.net>
---
 include/linux/iommu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c30d12e16473..f66f25051584 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1057,7 +1057,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
 
 static inline bool iommu_iotlb_gather_queued(struct iommu_iotlb_gather *gather)
 {
-	return gather && gather->queued;
+	return gather->queued;
 }
 
 static inline void iommu_dirty_bitmap_init(struct iommu_dirty_bitmap *dirty,
-- 
2.51.2.1041.gc1ab5b90ca-goog

On Mon, Nov 10, 2025 at 09:19:52PM -0800, Aashish Sharma wrote:
> The callers of 'iommu_iotlb_gather_queued' always have a non-null
> 'gather', so we don't need to check for the same in the function.

That's not obviously true to me, how did you check it?

For example, msm_iommu_pagetable_unmap() appears to call into
io_pgtable_ops::unmap_pages() with a NULL gather pointer.

Will

On 2025-11-24 5:03 pm, Will Deacon wrote:
> On Mon, Nov 10, 2025 at 09:19:52PM -0800, Aashish Sharma wrote:
>> The callers of 'iommu_iotlb_gather_queued' always have a non-null
>> 'gather', so we don't need to check for the same in the function.
> 
> That's not obviously true to me, how did you check it?
> 
> For example, msm_iommu_pagetable_unmap() appears to call into
> io_pgtable_ops::unmap_pages() with a NULL gather pointer.

Indeed I think the only redundant check is the one which seems to have 
snuck into __arm_lpae_unmap() not so long ago (which the other callers 
never had and still don't.)

Thanks,
Robin.

On Mon, Nov 24, 2025 at 9:16 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2025-11-24 5:03 pm, Will Deacon wrote:
> > On Mon, Nov 10, 2025 at 09:19:52PM -0800, Aashish Sharma wrote:
> >> The callers of 'iommu_iotlb_gather_queued' always have a non-null
> >> 'gather', so we don't need to check for the same in the function.
> >
> > That's not obviously true to me, how did you check it?
> >
> > For example, msm_iommu_pagetable_unmap() appears to call into
> > io_pgtable_ops::unmap_pages() with a NULL gather pointer.
>
> Indeed I think the only redundant check is the one which seems to have
> snuck into __arm_lpae_unmap() not so long ago (which the other callers
> never had and still don't.)

Apologies, you are correct; I missed the invocations where gather can
indeed be NULL. Please disregard the patch in its current form.

However, I'd like to share the motivation behind the change, as it
might point to a semantic fragility in the current API.

While auditing the ARM io-pgtable code, I noticed that whenever
iommu_iotlb_gather_queued() returns false, the code immediately calls
io_pgtable_tlb_add_page(), except in __arm_lpae_unmap(). This
eventually invokes iommu_flush_ops::tlb_add_page(). Many
implementations of this op assume gather is non-NULL and dereference
it directly.

This creates a confusing situation: iommu_iotlb_gather_queued()
returns false for two distinct conditions:
1. gather is NULL.
2. gather is valid but the queue is empty.

The callers, however, seem to implicitly assume the second case (empty
queue) and proceed to operations that would crash if gather were
actually NULL. For example, if __arm_v7s_unmap() is called with a NULL
gather, it can lead to a crash:

__arm_v7s_unmap -> io_pgtable_tlb_add_page ->
arm_smmu_tlb_inv_page_nosync -> iommu_iotlb_gather_add_page -> crash

While the lack of reported crashes suggests gather isn't NULL in these
specific paths, relying on iommu_iotlb_gather_queued() to handle NULL
pointers feels inherently risky given how the return value is
interpreted.

Do you think it would be valuable to refactor this so that the helper
strictly checks the queue status, making the callers explicitly
responsible for NULL checks, like in __arm_lpae_unmap()?

Thanks
Aashish