linux-kernel - Re: [PATCH v10 04/12] iommu: Add attach/detach_dev

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: <20220723141118.GD79279@nvidia.com>
Date:   Sat, 23 Jul 2022 11:11:18 -0300
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>,
        Kevin Tian <kevin.tian@...el.com>,
        Ashok Raj <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Vinod Koul <vkoul@...nel.org>,
        Eric Auger <eric.auger@...hat.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        Zhangfei Gao <zhangfei.gao@...aro.org>,
        Zhu Tony <tony.zhu@...el.com>, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Jean-Philippe Brucker <jean-philippe@...aro.org>
Subject: Re: [PATCH v10 04/12] iommu: Add attach/detach_dev_pasid iommu
 interface

On Tue, Jul 05, 2022 at 01:07:02PM +0800, Lu Baolu wrote:
> Attaching an IOMMU domain to a PASID of a device is a generic operation
> for modern IOMMU drivers which support PASID-granular DMA address
> translation. Currently visible usage scenarios include (but not limited):
> 
>  - SVA (Shared Virtual Address)
>  - kernel DMA with PASID
>  - hardware-assist mediated device
> 
> This adds a pair of domain ops for this purpose and adds the interfaces
> for device drivers to attach/detach a domain to/from a {device, PASID}.
> Some buses, like PCI, route packets without considering the PASID
> value.

Below the comments touch on ACS, so this is a bit out of date

> +static bool iommu_group_immutable_singleton(struct iommu_group *group,
> +					    struct device *dev)
> +{
> +	int count;
> +
> +	mutex_lock(&group->mutex);
> +	count = iommu_group_device_count(group);
> +	mutex_unlock(&group->mutex);
> +
> +	if (count != 1)
> +		return false;
> +
> +	/*
> +	 * The PCI device could be considered to be fully isolated if all
> +	 * devices on the path from the device to the host-PCI bridge are
> +	 * protected from peer-to-peer DMA by ACS.
> +	 */
> +	if (dev_is_pci(dev))
> +		return pci_acs_path_enabled(to_pci_dev(dev), NULL,
> +					    REQ_ACS_FLAGS);

You might want to explain what condition causes ACS isolated devices
to share a group in the first place..

> +
> +	/*
> +	 * Otherwise, the device came from DT/ACPI, assume it is static and
> +	 * then singleton can know from the device count in the group.
> +	 */
> +	return true;
> +}

I would be happer if probe was changed to refuse to add a device to a
group if the group's pasid xarray is not empty, as a protective
measure.

> +int iommu_attach_device_pasid(struct iommu_domain *domain, struct device *dev,
> +			      ioasid_t pasid)
> +{
> +	struct iommu_group *group;
> +	void *curr;
> +	int ret;
> +
> +	if (!domain->ops->set_dev_pasid)
> +		return -EOPNOTSUPP;
> +
> +	group = iommu_group_get(dev);
> +	if (!group || !iommu_group_immutable_singleton(group, dev)) {
> +		iommu_group_put(group);
> +		return -EINVAL;

goto error below

> +	}
> +
> +	mutex_lock(&group->mutex);

Just hold the group->mutex a few lines above and don't put locking in
iommu_group_immutable_singleton(), it is clearer

> +void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
> +			       ioasid_t pasid)
> +{
> +	struct iommu_group *group = iommu_group_get(dev);
> +
> +	mutex_lock(&group->mutex);
> +	domain->ops->block_dev_pasid(domain, dev, pasid);

I still really this OP, it is nonsense to invoke 'block_dev_pasid' on
a domain, it should be on the iommu ops and it should not take in a
domain parameter. This is why I prefer we write it as

domain->ops->set_dev_pasid(group->blocking_domain, dev, pasid);

> +	xa_erase(&group->pasid_array, pasid);

It is worth checking that the value returned from xa_erase is domain
and WARN_ON if not, since we are passing domain in..

Jason