linux-kernel - Re: [PATCH rfcv2 6/8] iommu/arm-smmu-v3: Populate smmu

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: <aNw7UVaktL6/05b8@Asurada-Nvidia>
Date: Tue, 30 Sep 2025 13:19:29 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
	<jean-philippe@...aro.org>, <miko.lenczewski@....com>, <balbirs@...dia.com>,
	<peterz@...radead.org>, <smostafa@...gle.com>, <kevin.tian@...el.com>,
	<praan@...gle.com>, <linux-arm-kernel@...ts.infradead.org>,
	<iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH rfcv2 6/8] iommu/arm-smmu-v3: Populate smmu_domain->invs
 when attaching masters

On Tue, Sep 30, 2025 at 09:12:00AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 29, 2025 at 01:52:30PM -0700, Nicolin Chen wrote:
> 
> > > > +	if (!new_invs) {
> > > > +		size_t new_num = old_invs->num_invs;
> > > > +
> > > > +		/*
> > > > +		 * OOM. Couldn't make a copy. Leave the array unoptimized. But
> > > > +		 * trim its size if some tailing entries are marked as trash.
> > > > +		 */
> > > > +		while (new_num != 0) {
> > > > +			if (refcount_read(&old_invs->inv[new_num - 1].users))
> > > > +				break;
> > > > +			new_num--;
> > > > +		}
> > > 
> > > Would be nicer to have arm_smmu_invs_unref return the new size so we
> > > don't need this loop
> > 
> > The "new size" must be invs->num_invs subtracting the number of
> > the tailing trash entries. So, arm_smmu_invs_unref() would have
> > to have the same loop validating the tailing entries, right?
> 
> It doesn't need another loop, it just need to record the index of the
> last valid entry while it is doing its own loop. If it reaches
> invs->num_invs then that will be the new length.

arm_smmu_invs_purge() needs num_dels, while its fallback routine
needs num_invs (new size). This forces arm_smmu_invs_unref() to
return two numbers.

I see a cleaner way of handling this is to update invs->num_invs
inside arm_smmu_invs_unref():
----------------------------------------------------------------
@@ -1209,6 +1216,13 @@ size_t arm_smmu_invs_unref(struct arm_smmu_invs *invs,
                        j++;
                }
        }
+
+       /* The lock is required to fence concurrent ATS operations. */
+       write_lock_irqsave(&invs->rwlock, flags);
+       /* Trim the size by removing tailing trash entries */
+       WRITE_ONCE(invs->num_invs, num_invs);
+       write_unlock_irqrestore(&invs->rwlock, flags);
+
        return num_dels;
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_invs_unref);
----------------------------------------------------------------

So the caller would look like:
----------------------------------------------------------------
	num_dels = arm_smmu_invs_unref(old_invs, invst->new_invs);
	if (!num_dels)
		return;
 
	new_invs = arm_smmu_invs_purge(old_invs, num_dels);
	if (!new_invs)
		return;
----------------------------------------------------------------

Thanks
Nicolin