Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions

Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions

[Jean-Philippe Brucker]

Hi Nicolin,

On Tue, Sep 13, 2022 at 01:24:47AM -0700, Nicolin Chen wrote:
> Following the new rules in include/linux/iommu.h kdocs, update all drivers
> ->attach_dev callback functions to return ENODEV error code for all device
> specific errors. It particularly excludes EINVAL from being used for such
> error cases. For the same purpose, also replace one EINVAL with ENOMEM in
> mtk_iommu driver.
> 
> Note that the virtio-iommu does a viommu_domain_map_identity() call, which
> returns either 0 or ENOMEM at this moment. Change to "return ret" directly
> to allow it to pass an EINVAL in the future.
[...]
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 80151176ba12..874c01634d2b 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -696,7 +696,7 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
>  		if (ret) {
>  			ida_free(&viommu->domain_ids, vdomain->id);
>  			vdomain->viommu = NULL;
> -			return -EOPNOTSUPP;
> +			return ret;

I think in the future it will be too easy to forget about the constrained
return value of attach() while modifying some other part of the driver,
and let an external helper return EINVAL. So I'd rather not propagate ret
from outside of viommu_domain_attach() and finalise().

For the same reason I do prefer this solution over EMEDIUMTYPE, because
it's too tempting to use exotic errno when they seem appropriate instead
of boring ENODEV and EINVAL. The alternative would be adding a special
purpose code to linux/errno.h, similarly to EPROBE_DEFER, but that might
be excessive.

Since we can't guarantee that APIs like virtio or ida won't ever return
EINVAL, we should set all return values:

--- 8< ---
From 7b16796cb78d11971236f98fd2d3cd73ca769827 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Brucker <jean-philippe@linaro.org>
Date: Tue, 13 Sep 2022 12:53:02 +0100
Subject: [PATCH] iommu/virtio: Constrain return value of viommu_attach_dev()

Ensure viommu_attach_dev() only return errno values expected from the
attach_dev() op. In particular, only return EINVAL when we're sure that
the device is incompatible with the domain.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 08eeafc9529f..582ff5a33b52 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -669,13 +669,13 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
 		dev_err(vdev->dev,
 			"granule 0x%lx larger than system page size 0x%lx\n",
 			viommu_page_size, PAGE_SIZE);
-		return -EINVAL;
+		return -ENODEV;
 	}
 
 	ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain,
 			      viommu->last_domain, GFP_KERNEL);
 	if (ret < 0)
-		return ret;
+		return -ENOMEM;
 
 	vdomain->id		= (unsigned int)ret;
 
@@ -696,7 +696,7 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
 		if (ret) {
 			ida_free(&viommu->domain_ids, vdomain->id);
 			vdomain->viommu = NULL;
-			return -EOPNOTSUPP;
+			return -ENODEV;
 		}
 	}
 
@@ -734,7 +734,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		ret = viommu_domain_finalise(vdev, domain);
 	} else if (vdomain->viommu != vdev->viommu) {
 		dev_err(dev, "cannot attach to foreign vIOMMU\n");
-		ret = -EXDEV;
+		ret = -EINVAL;
 	}
 	mutex_unlock(&vdomain->mutex);
 
@@ -769,7 +769,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 		ret = viommu_send_req_sync(vdomain->viommu, &req, sizeof(req));
 		if (ret)
-			return ret;
+			return -ENODEV;
 	}
 
 	if (!vdomain->nr_endpoints) {
@@ -779,7 +779,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		 */
 		ret = viommu_replay_mappings(vdomain);
 		if (ret)
-			return ret;
+			return -ENODEV;
 	}
 
 	vdomain->nr_endpoints++;
-- 
2.37.3

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions

[Jean-Philippe Brucker]

On Wed, Sep 14, 2022 at 06:11:06AM -0300, Jason Gunthorpe wrote:
> On Tue, Sep 13, 2022 at 01:27:03PM +0100, Jean-Philippe Brucker wrote:
> > I think in the future it will be too easy to forget about the constrained
> > return value of attach() while modifying some other part of the driver,
> > and let an external helper return EINVAL. So I'd rather not propagate ret
> > from outside of viommu_domain_attach() and finalise().
> 
> Fortunately, if -EINVAL is wrongly returned it only creates an
> inefficiency, not a functional problem. So we do not need to be
> precise here.

Ah fair. In that case the attach_dev() documentation should indicate that
EINVAL is a hint, so that callers don't rely on it (currently words "must"
and "exclusively" indicate that returning EINVAL for anything other than
device-domain incompatibility is unacceptable). The virtio-iommu
implementation may well return EINVAL from the virtio stack or from the
host response.

Thanks,
Jean

> 
> > Since we can't guarantee that APIs like virtio or ida won't ever return
> > EINVAL, we should set all return values:
> 
> I dislike this alot, it squashes all return codes to try to optimize
> an obscure failure path :(
> 
> Jason
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions

[Robin Murphy]

On 2022-09-14 18:58, Nicolin Chen wrote:
> On Wed, Sep 14, 2022 at 10:49:42AM +0100, Jean-Philippe Brucker wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Wed, Sep 14, 2022 at 06:11:06AM -0300, Jason Gunthorpe wrote:
>>> On Tue, Sep 13, 2022 at 01:27:03PM +0100, Jean-Philippe Brucker wrote:
>>>> I think in the future it will be too easy to forget about the constrained
>>>> return value of attach() while modifying some other part of the driver,
>>>> and let an external helper return EINVAL. So I'd rather not propagate ret
>>>> from outside of viommu_domain_attach() and finalise().
>>>
>>> Fortunately, if -EINVAL is wrongly returned it only creates an
>>> inefficiency, not a functional problem. So we do not need to be
>>> precise here.
>>
>> Ah fair. In that case the attach_dev() documentation should indicate that
>> EINVAL is a hint, so that callers don't rely on it (currently words "must"
>> and "exclusively" indicate that returning EINVAL for anything other than
>> device-domain incompatibility is unacceptable). The virtio-iommu
>> implementation may well return EINVAL from the virtio stack or from the
>> host response.
> 
> How about this?
> 
> + * * EINVAL    - mainly, device and domain are incompatible, or something went
> + *               wrong with the domain. It's suggested to avoid kernel prints
> + *               along with this errno. And it's better to convert any EINVAL
> + *               returned from kAPIs to ENODEV if it is device-specific, or to
> + *               some other reasonable errno being listed below

FWIW, I'd say something like:

"The device and domain are incompatible. If this is due to some previous 
configuration of the domain, drivers should not log an error, since it 
is legitimate for callers to test reuse of an existing domain. 
Otherwise, it may still represent some fundamental problem."

And then at the public interfaces state it from other angle:

"The device and domain are incompatible. If the domain has already been 
used or configured in some way, attaching the same device to a different 
domain may be expected to succeed. Otherwise, it may still represent 
some fundamental problem."

[ and to save another mail, I'm not sure copying the default comment for 
ENOSPC is all that helpful either - what is "space" for something that 
isn't a storage device? I'd guess limited hardware resources in some 
form, but in the IOMMU context, potential confusion with address space 
is maybe a little too close for comfort? ]

>>>> Since we can't guarantee that APIs like virtio or ida won't ever return
>>>> EINVAL, we should set all return values:
>>>
>>> I dislike this alot, it squashes all return codes to try to optimize
>>> an obscure failure path :(
> 
> Hmm...should I revert all the driver changes back to this version?

Yeah, I don't think we need to go too mad here. Drivers shouldn't emit 
their *own* -EINVAL unless appropriate, but if it comes back from some 
external API then that implies something's gone unexpectedly wrong 
anyway - maybe it's a transient condition and a subsequent different 
attach might actually work out OK? We can't really say in general. 
Besides, if the driver sees an error which implies it's done something 
wrong itself, it probably shouldn't be trusted to try to reason about it 
further. The caller can handle any error as long as we set their 
expectations correctly.

Thanks,
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization