Re: [PATCH V2 3/4] iommufd: Add IOMMU_IOAS_CHANGE_PROCESS

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Sep 26, 2024 at 06:53:47AM -0700, Steve Sistare wrote:

> +static int charge_current(unsigned long *npinned)
> +{
> +	struct iopt_pages tmp = {
> +		.source_mm = current->mm,
> +		.source_task = current->group_leader,
> +		.source_user = current_user(),
> +	};
> +	unsigned int account_mode;
> +	int rc;

This seems like it needs a bit more protection, we shouldn't be
touching pages that are not set to current otherwise it will corrupt
the counts during the uncharge?

Maybe the whole operation should fail if any areas are not owned by
this process?

> +	for (account_mode = 0; account_mode != IOPT_PAGES_ACCOUNT_MODE_NUM;
> +	     account_mode++) {
> +		if (!npinned[account_mode])
> +			continue;
> +
> +		tmp.account_mode = account_mode;
> +		rc = iopt_update_pinned(&tmp, npinned[account_mode], true,
> +					NULL);
> +		if (rc)
> +			goto err_undo;
> +	}
> +	return 0;
> +
> +err_undo:
> +	while (account_mode != 0) {
> +		account_mode--;

Do we need to check:

		if (!npinned[account_mode])
			continue;

To match the charging side?

> +int iommufd_ioas_change_process(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_ioas_change_process *cmd = ucmd->cmd;
> +	struct iommufd_ctx *ictx = ucmd->ictx;
> +	unsigned long all_npinned[IOPT_PAGES_ACCOUNT_MODE_NUM] = {0, 0, 0};

Just use  "= {}"

> +	struct iommufd_ioas *ioas;
> +	struct iopt_area *area;
> +	struct iopt_pages *pages;
> +	struct xarray ioas_list;
> +	unsigned long index;
> +	int rc;
> +
> +	if (cmd->__reserved)
> +		return -EOPNOTSUPP;
> +
> +	xa_init(&ioas_list);
> +	rc = iommufd_take_all_iova_rwsem(ictx, &ioas_list);
> +	if (rc)
> +		goto out_no_release;
> +
> +	for_each_ioas_area(&ioas_list, index, ioas, area)  {
> +		if (area->pages->type != IOPT_ADDRESS_FILE) {
> +			rc = -EINVAL;
> +			goto out;
> +		}
> +	}
> +
> +	/*
> +	 * Count last_pinned pages, then clear it to avoid double counting
> +	 * if the same iopt_pages is visited multiple times in this loop.
> +	 * Since we are under all the locks, npinned == last_npinned, so we
> +	 * can easily restore last_npinned before we return.
> +	 */
> +	for_each_ioas_area(&ioas_list, index, ioas, area)  {
> +		pages = area->pages;
> +
> +		if (need_charge_update(pages)) {
> +			all_npinned[pages->account_mode] += pages->last_npinned;
> +			pages->last_npinned = 0;
> +		}
> +	}
> +
> +	rc = charge_current(all_npinned);
> +
> +	if (rc) {
> +		/* Charge failed.  Fix last_npinned and bail. */
> +		for_each_ioas_area(&ioas_list, index, ioas, area)
> +			area->pages->last_npinned = area->pages->npinned;
> +		goto out;
> +	}
> +
> +	for_each_ioas_area(&ioas_list, index, ioas, area) {
> +		pages = area->pages;
> +
> +		/* Uncharge the old one (which also restores last_npinned) */
> +		if (need_charge_update(pages))
> +			iopt_update_pinned(pages, pages->npinned, false, NULL);
> +
> +		change_mm(pages);
> +	}
> +
> +out:
> +	iommufd_release_all_iova_rwsem(ictx, &ioas_list);
> +
> +out_no_release:
> +	return rc;

Don't need this label

Jason