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

Robin Murphy
2026-03-03T15:56:43Z

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.