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]
Message-ID: <20220209134152.GA4160@nvidia.com>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ