linux-kernel - Re: [PATCH v5 3/5] iommu: Add verisilicon IOMMU driver

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] [day] [month] [year] [list]

Message-ID: <2759380e-51a5-4437-8caa-0039ccd79559@collabora.com>
Date: Thu, 10 Jul 2025 10:26:24 +0200
From: Benjamin Gaignard <benjamin.gaignard@...labora.com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: joro@...tes.org, will@...nel.org, robin.murphy@....com, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, heiko@...ech.de,
 nicolas.dufresne@...labora.com, iommu@...ts.linux.dev,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 kernel@...labora.com
Subject: Re: [PATCH v5 3/5] iommu: Add verisilicon IOMMU driver


Le 09/07/2025 à 18:45, Jason Gunthorpe a écrit :
> On Wed, Jul 09, 2025 at 10:53:28AM +0200, Benjamin Gaignard wrote:
>> +static int vsi_iommu_attach_device(struct iommu_domain *domain,
>> +				   struct device *dev)
>> +{
>> +	struct vsi_iommu *iommu = dev_iommu_priv_get(dev);
>> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	ret = pm_runtime_resume_and_get(iommu->dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	spin_lock_irqsave(&iommu->lock, flags);
>> +	/* iommu already attached */
>> +	if (iommu->domain == domain)
>> +		goto unlock;
>> +
>> +	vsi_iommu_enable(iommu, domain);
>> +	list_add_tail(&iommu->node, &vsi_domain->iommus);
>> +	iommu->domain = domain;
>> +
>> +unlock:
>> +	spin_unlock_irqrestore(&iommu->lock, flags);
>> +	pm_runtime_put_autosuspend(iommu->dev);
>> +	return ret;
> I thought this was mentioned before, but this doesn't handle
> attach_device being called twice without an identity attach in
> between.
>
> And now the new locking doesn't protect concurrent invalidation races,
> the lock is in the wrong place.
>
> hold the domain lock across the whole sequence to hold off any
> invalidation until the linked list is consistent with the HW
> programming:
>
>   spin_lock_irqsave(&vsi_domain->lock, flags2); // Prevent invalidation
>
>   vsi_iommu_enable(iommu, domain);
>   list_del(&iommu->node);
>   list_add_tail(&iommu->node, &vsi_domain->iommus);
>
>   spin_unlock_irqrestore(&vsi_domain->lock, flags);
>
> Then remove this:
>
>   +	/* iommu already attached */
>   +	if (iommu->domain == domain)
>   +		goto unlock;
>
> Since the fix above makes it safe regardless.
>
> And, also feels like again, but vsi_iommu_enable() needs to fully
> flush the cache since the translation is being changed, shouldn't
> there also be writes to VSI_MMU_FLUSH_BASE ?
>
> Otherwise the locking change looks OK and I don't have other comments.

I have fix that in v6.

Thanks a lot for your review.

Benjamin

>
> Jason