linux-kernel - Re: [PATCH v2 09/10] iommu: Use dev_iommu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Feb 2022 09:41:52 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     Joerg Roedel <joro@...tes.org>,
        Christoph Hellwig <hch@...radead.org>,
        Ben Skeggs <bskeggs@...hat.com>,
        Kevin Tian <kevin.tian@...el.com>,
        Ashok Raj <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Eric Auger <eric.auger@...hat.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 09/10] iommu: Use dev_iommu_ops() helper

On Tue, Feb 08, 2022 at 09:25:58AM +0800, Lu Baolu wrote:
> Convert all the feasible instances of dev->bus->iommu_ops to
> dev_iommu_ops() in order to making the operation of obtaining
> iommu_ops from a device consistent.

Why are there two patches doing this conversion? Roll this into the
prior patch?

>  void iommu_get_resv_regions(struct device *dev, struct list_head *list)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>  
>  	if (ops && ops->get_resv_regions)
>  		ops->get_resv_regions(dev, list);

And agree with Christoph, don't keep confusing ops null tests -
dev_iommu_ops() never returns null and any function using it must rely
on the caller to handle this, somehow.

However, I wonder how safe this is. Certainly some APIs like this one
it is fine, but I would be happier if the 'first' APIs like
bind/attach/alloc/etc fail silently upwards. In many cases these APIs
are called based on things like DT configuration, or VFIO or
something, and the caller is expecting the iommu layer to do all
necessary validation.

> @@ -2788,7 +2789,7 @@ iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
>  {
>  	struct iommu_group *group;
>  	struct iommu_sva *handle = ERR_PTR(-EINVAL);
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>  
>  	if (!ops || !ops->sva_bind)
>  		return ERR_PTR(-ENODEV);
> @@ -2831,7 +2832,7 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
>  {
>  	struct iommu_group *group;
>  	struct device *dev = handle->dev;
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>  
>  	if (!ops || !ops->sva_unbind)
>  		return;
> @@ -2850,7 +2851,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
>  
>  u32 iommu_sva_get_pasid(struct iommu_sva *handle)
>  {
> -	const struct iommu_ops *ops = handle->dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(handle->dev);
>  
>  	if (!ops || !ops->sva_get_pasid)
>  		return IOMMU_PASID_INVALID;

We all agreed that this sva object will turn into a domain and thus
all of this will eventually move to domain ops?

Jason