linux-kernel - Re: [PATCH rfcv1 4/8] iommu/arm-smmu-v3: Introduce a per-domain arm_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: <20250826195003.GA2151485@nvidia.com>
Date: Tue, 26 Aug 2025 16:50:03 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Nicolin Chen <nicolinc@...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, zhangzekun11@...wei.com,
	linux-arm-kernel@...ts.infradead.org, iommu@...ts.linux.dev,
	linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH rfcv1 4/8] iommu/arm-smmu-v3: Introduce a per-domain
 arm_smmu_invs array

On Wed, Aug 13, 2025 at 06:25:35PM -0700, Nicolin Chen wrote:
> +/**
> + * arm_smmu_invs_add() - Combine @old_invs with @add_invs to a new array
> + * @old_invs: the old invalidation array
> + * @add_invs: an array of invlidations to add
> + *
> + * Return: a newly allocated and sorted invalidation array on success, or an
> + * ERR_PTR.
> + *
> + * This function must be locked and serialized with arm_smmu_invs_del/dec(),
> + * but do not lockdep on any lock for KUNIT test.
> + *
> + * Caller is resposible for freeing the @old_invs and the returned one.
> + *
> + * Entries marked as trash can be resued if @add_invs wants to add them back.
> + * Otherwise, they will be completely removed in the returned array.
> + */
> +VISIBLE_IF_KUNIT
> +struct arm_smmu_invs *arm_smmu_invs_add(struct arm_smmu_invs *old_invs,
> +					struct arm_smmu_invs *add_invs)
> +{
> +	size_t need = old_invs->num_invs + add_invs->num_invs;
> +	struct arm_smmu_invs *new_invs;
> +	size_t deletes = 0, i, j;
> +	u64 existed = 0;
> +
> +	/* Max of add_invs->num_invs is 64 */
> +	if (WARN_ON(add_invs->num_invs > sizeof(existed) * 8))
> +		return ERR_PTR(-EINVAL);

Since this is driven off of num_streams using a fixed bitmap doesn't
seem great since I suppose the dt isn't limited to 64.

Given how this is working now I think you can just add a new member to
the struct:

struct arm_smmu_inv {
	/* invalidation items */
	struct arm_smmu_device *smmu;
	u8 type;
	u8 size_opcode;
	u8 nsize_opcode;
	/* Temporary bits for add/del functions */
	u8 reuse:1;
	u8 todel:1;

And use reuse as the temporary instead of the bitmap.

> +	for (i = 0; i != old_invs->num_invs; i++) {
> +		struct arm_smmu_inv *cur = &old_invs->inv[i];

missing newline

> +		/* Count the trash entries to deletes */
> +		if (cur->todel) {
> +			WARN_ON_ONCE(refcount_read(&cur->users));
> +			deletes++;
> +		}

Just do continue here.

todel should only be used as a temporary. Use refcount_read() ==
0. Then you don't need a WARN either.

> +		for (j = 0; j != add_invs->num_invs; j++) {
> +			if (!same_op(cur, &add_invs->inv[j]))
> +				continue;
> +			/* Found duplicated entries in add_invs */
> +			if (WARN_ON_ONCE(existed & BIT_ULL(j)))
> +				continue;

inv[j].reuse

> +			/* Revert the todel marker for reuse */
> +			if (cur->todel) {
> +				cur->todel = false;
> +				deletes--;

This wil blow up the refcount_inc() below because users is 0..
There is no point in trying to optimize like this just discard the
old entry and add a new one.

> +	new_invs = arm_smmu_invs_alloc(need);
> +	if (IS_ERR(new_invs)) {
> +		/* Don't forget to revert all the todel markers */
> +		for (i = 0; i != old_invs->num_invs; i++) {
> +			if (refcount_read(&old_invs->inv[i].users) == 0)
> +				old_invs->inv[i].todel = true;
> +		}

Drop as well

> +		return new_invs;
> +	}
> +
> +	/* Copy the entire array less all the todel entries */
> +	for (i = 0; i != old_invs->num_invs; i++) {
> +		if (old_invs->inv[i].todel)
> +			continue;

refcount_read

> +		new_invs->inv[new_invs->num_invs++] = old_invs->inv[i];
> +	}
> +
> +	for (j = 0; j != add_invs->num_invs; j++) {
> +		if (existed & BIT_ULL(j)) {

inv[j]->reuse

> +			unsigned int idx = add_invs->inv[j].id;


Similar remarks for del, use users to set todel, don't expect it to be
valid coming into the function.

Jason