| 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
| ||
|
Message-ID: <1b4c7742-1e0e-42c4-be52-c5fa55c24ca0@nvidia.com>
Date: Tue, 13 May 2025 18:51:20 -0700
From: Tushar Dave <tdave@...dia.com>
To: Yi Liu <yi.l.liu@...el.com>, "joro@...tes.org" <joro@...tes.org>,
"will@...nel.org" <will@...nel.org>,
"robin.murphy@....com" <robin.murphy@....com>,
"kevin.tian@...el.com" <kevin.tian@...el.com>,
Jason Gunthorpe <jgg@...dia.com>,
"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v3 rc] iommu: Skip PASID validation for devices without
PASID capability
On 5/7/25 17:21, Tushar Dave wrote:
>
>
> On 5/7/25 06:59, Yi Liu wrote:
>>
>> On 2025/5/6 05:15, Tushar Dave wrote:
>>> Generally PASID support requires ACS settings that usually create
>>> single device groups, but there are some niche cases where we can get
>>> multi-device groups and still have working PASID support. The primary
>>> issue is that PCI switches are not required to treat PASID tagged TLPs
>>> specially so appropriate ACS settings are required to route all TLPs to
>>> the host bridge if PASID is going to work properly.
>>>
>>> pci_enable_pasid() does check that each device that will use PASID has
>>> the proper ACS settings to achieve this routing.
>>>
>>> However, no-PASID devices can be combined with PASID capable devices
>>> within the same topology using non-uniform ACS settings. In this case
>>> the no-PASID devices may not have strict route to host ACS flags and
>>> end up being grouped with the PASID devices.
>>>
>>> This configuration fails to allow use of the PASID within the iommu
>>> core code which wrongly checks if the no-PASID device supports PASID.
>>>
>>> Fix this by ignoring no-PASID devices during the PASID validation. They
>>> will never issue a PASID TLP anyhow so they can be ignored.
>>>
>>> Fixes: c404f55c26fc ("iommu: Validate the PASID in iommu_attach_device_pasid()")
>>> Cc: stable@...r.kernel.org
>>> Signed-off-by: Tushar Dave <tdave@...dia.com>
>>> ---
>>>
>>> changes in v3:
>>> - addressed review comment from Vasant.
>>>
>>> drivers/iommu/iommu.c | 27 +++++++++++++++++++--------
>>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 60aed01e54f2..636fc68a8ec0 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -3329,10 +3329,12 @@ static int __iommu_set_group_pasid(struct iommu_domain
>>> *domain,
>>> int ret;
>>> for_each_group_device(group, device) {
>>> - ret = domain->ops->set_dev_pasid(domain, device->dev,
>>> - pasid, NULL);
>>> - if (ret)
>>> - goto err_revert;
>>> + if (device->dev->iommu->max_pasids > 0) {
>>> + ret = domain->ops->set_dev_pasid(domain, device->dev,
>>> + pasid, NULL);
>>> + if (ret)
>>> + goto err_revert;
>>> + }
>>> }
>>> return 0;
>>> @@ -3342,7 +3344,8 @@ static int __iommu_set_group_pasid(struct iommu_domain
>>> *domain,
>>> for_each_group_device(group, device) {
>>> if (device == last_gdev)
>>> break;
>>> - iommu_remove_dev_pasid(device->dev, pasid, domain);
>>> + if (device->dev->iommu->max_pasids > 0)
>>> + iommu_remove_dev_pasid(device->dev, pasid, domain);
>>
>> Reviewed-by: Yi Liu <yi.l.liu@...el.com>
>>
>> with a nit. would it save some loc by adding the max_pasids check in
>> iommu_remove_dev_pasid()?
>
> With current code:
>
> drivers/iommu/iommu.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
>
> If I move the pasid check in iommu_remove_dev_pasid(), it would be:
>
> drivers/iommu/iommu.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
Yi Liu,
Should I send v4 with the change below or we are good with v3?
-Tushar
>
> e.g.
>
> @@ -3318,8 +3318,9 @@ static void iommu_remove_dev_pasid(struct device *dev,
> ioasid_t pasid,
> const struct iommu_ops *ops = dev_iommu_ops(dev);
> struct iommu_domain *blocked_domain = ops->blocked_domain;
>
> - WARN_ON(blocked_domain->ops->set_dev_pasid(blocked_domain,
> - dev, pasid, domain));
> + if (dev->iommu->max_pasids > 0)
> + WARN_ON(blocked_domain->ops->set_dev_pasid(blocked_domain,
> + dev, pasid, domain));
> }
>
> static int __iommu_set_group_pasid(struct iommu_domain *domain,
> @@ -3329,10 +3330,12 @@ static int __iommu_set_group_pasid(struct iommu_domain
> *domain,
> int ret;
>
> for_each_group_device(group, device) {
> - ret = domain->ops->set_dev_pasid(domain, device->dev,
> - pasid, NULL);
> - if (ret)
> - goto err_revert;
> + if (device->dev->iommu->max_pasids > 0) {
> + ret = domain->ops->set_dev_pasid(domain, device->dev,
> + pasid, NULL);
> + if (ret)
> + goto err_revert;
> + }
> }
>
> return 0;
>
> Last hunk remain same as before for iommu_attach_device_pasid()
>
>
> Let me know.
>
> -Tushar
>
>
>>
>>
>>> }
>>> return ret;
>>> }
>>> @@ -3353,8 +3356,10 @@ static void __iommu_remove_group_pasid(struct
>>> iommu_group *group,
>>> {
>>> struct group_device *device;
>>> - for_each_group_device(group, device)
>>> - iommu_remove_dev_pasid(device->dev, pasid, domain);
>>> + for_each_group_device(group, device) {
>>> + if (device->dev->iommu->max_pasids > 0)
>>> + iommu_remove_dev_pasid(device->dev, pasid, domain);
>>> + }
>>> }
>>> /*
>>> @@ -3391,7 +3396,13 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>> mutex_lock(&group->mutex);
>>> for_each_group_device(group, device) {
>>> - if (pasid >= device->dev->iommu->max_pasids) {
>>> + /*
>>> + * Skip PASID validation for devices without PASID support
>>> + * (max_pasids = 0). These devices cannot issue transactions
>>> + * with PASID, so they don't affect group's PASID usage.
>>> + */
>>> + if ((device->dev->iommu->max_pasids > 0) &&
>>> + (pasid >= device->dev->iommu->max_pasids)) {
>>> ret = -EINVAL;
>>> goto out_unlock;
>>> }
>>
>
Powered by blists - more mailing lists