Re: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Robin Murphy]

On 2022-09-08 01:43, Jason Gunthorpe wrote:
> On Wed, Sep 07, 2022 at 08:41:13PM +0100, Robin Murphy wrote:
>
> > > > FWIW, we're now very close to being able to validate dev->iommu against
> > > > where the domain came from in core code, and so short-circuit ->attach_dev
> > > > entirely if they don't match.
> > >
> > > I don't think this is a long term direction. We have systems now with
> > > a number of SMMU blocks and we really are going to see a need that
> > > they share the iommu_domains so we don't have unncessary overheads
> > > from duplicated io page table memory.
> > >
> > > So ultimately I'd expect to pass the iommu_domain to the driver and
> > > the driver will decide if the page table memory it represents is
> > > compatible or not. Restricting to only the same iommu instance isn't
> > > good..
> >
> > Who said IOMMU instance?
>
> Ah, I completely misunderstood what 'dev->iommu' was referring too, OK
> I see.
>
> > Again, not what I was suggesting. In fact the nature of iommu_attach_group()
> > already rules out bogus devices getting this far, so all a driver currently
> > has to worry about is compatibility of a device that it definitely probed
> > with a domain that it definitely allocated. Therefore, from a caller's point
> > of view, if attaching to an existing domain returns -EINVAL, try another
> > domain; multiple different existing domains can be tried, and may also
> > return -EINVAL for the same or different reasons; the final attempt is to
> > allocate a fresh domain and attach to that, which should always be nominally
> > valid and *never* return -EINVAL. If any attempt returns any other error,
> > bail out down the usual "this should have worked but something went wrong"
> > path. Even if any driver did have a nonsensical "nothing went wrong, I just
> > can't attach my device to any of my domains" case, I don't think it would
> > really need distinguishing from any other general error anyway.
>
> The algorithm you described is exactly what this series does, it just
> used EMEDIUMTYPE instead of EINVAL. Changing it to EINVAL is not a
> fundamental problem, just a bit more work.
>
> Looking at Nicolin's series there is a bunch of existing errnos that
> would still need converting, ie EXDEV, EBUSY, EOPNOTSUPP, EFAULT, and
> ENXIO are all returned as codes for 'domain incompatible with device'
> in various drivers. So the patch would still look much the same, just
> changing them to EINVAL instead of EMEDIUMTYPE.
>
> That leaves the question of the remaining EINVAL's that Nicolin did
> not convert to EMEDIUMTYPE.
>
> eg in the AMD driver:
>
> 	if (!check_device(dev))
> 		return -EINVAL;
>
> 	iommu = rlookup_amd_iommu(dev);
> 	if (!iommu)
> 		return -EINVAL;
>
> These are all cases of 'something is really wrong with the device or
> iommu, everything will fail'. Other drivers are using ENODEV for this
> already, so we'd probably have an additional patch changing various
> places like that to ENODEV.
>
> This mixture of error codes is the basic reason why a new code was
> used, because none of the existing codes are used with any
> consistency.
>
> But OK, I'm on board, lets use more common errnos with specific
> meaning, that can be documented in a comment someplace:
>   ENOMEM - out of memory
>   ENODEV - no domain can attach, device or iommu is messed up
>   EINVAL - the domain is incompatible with the device
>   <others> - Same behavior as ENODEV, use is discouraged.
>
> I think achieving consistency of error codes is a generally desirable
> goal, it makes the error code actually useful.
>
> Joerg this is a good bit of work, will you be OK with it?
>
> > Thus as long as we can maintain that basic guarantee that attaching
> > a group to a newly allocated domain can only ever fail for resource
> > allocation reasons and not some spurious "incompatibility", then we
> > don't need any obscure trickery, and a single, clear, error code is
> > in fact enough to say all that needs to be said.
>
> As above, this is not the case, drivers do seem to have error paths
> that are unconditional on the domain. Perhaps they are just protective
> assertions and never happen.

Right, that's the gist of what I was getting at - I think it's worth 
putting in the effort to audit and fix the drivers so that that *can* be 
the case, then we can have a meaningful error API with standard codes 
effectively for free, rather than just sighing at the existing mess and 
building a slightly esoteric special case on top.

Case in point, the AMD checks quoted above are pointless, since it 
checks the same things in ->probe_device, and if that fails then the 
device won't get a group so there's no way for it to even reach 
->attach_dev any more. I'm sure there's a *lot* of cruft that can be 
cleared out now that per-device and per-domain ops give us this kind of 
inherent robustness.

Cheers,
Robin.

> Regardless, it doesn't matter. If they return ENODEV or EINVAL the
> VFIO side algorithm will continue to work fine, it just does alot more
> work if EINVAL is permanently returned.
>
> Thanks,
> Jason