Re: [PATCH v4 1/7] iommu: Prevent pasid attach if no ops->remove_dev_pasid

[Baolu Lu <baolu.lu@linux.intel.com>]

On 11/8/24 20:04, Yi Liu wrote:
> driver should implement both set_dev_pasid and remove_dev_pasid op, otherwise
> it is a problem how to detach pasid. In reality, it is impossible that an
> iommu driver implements set_dev_pasid() but no remove_dev_pasid() op. However,
> it is better to check it.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 13fcd9d8f2df..1c689e57928e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3352,17 +3352,19 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>   			      struct iommu_attach_handle *handle)
>   {
>   	/* Caller must be a probed driver on dev */
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>   	struct iommu_group *group = dev->iommu_group;
>   	struct group_device *device;
>   	int ret;
>   
> -	if (!domain->ops->set_dev_pasid)
> +	if (!domain->ops->set_dev_pasid ||
> +	    !ops->remove_dev_pasid)
>   		return -EOPNOTSUPP;
>   
>   	if (!group)
>   		return -ENODEV;

If group is NULL, calling dev_iommu_ops() will trigger a kernel NULL
pointer reference warning, which is unintended. If you need to check
ops->remove_dev_pasid, it should be done after the group check.

>   
> -	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> +	if (!dev_has_iommu(dev) || ops != domain->owner ||
>   	    pasid == IOMMU_NO_PASID)
>   		return -EINVAL;
>   

--
baolu