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

Janusz Krzysztofik
2026-03-19T10:02:24Z

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.