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

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

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.