Re: [PATCH 07/26] dma-buf: Add dma_buf_mapping_map() returning a type-tagged mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dma-buf: do not clear prior attach->selected on error in dma_buf_mapping_map()
--

commit 03dc2c734a4965633dec9f21493634bacf3d9de7
Author: Alistair Popple <apopple@nvidia.com>

dma-buf: Add dma_buf_mapping_map() returning a type-tagged mapping

This commit introduces dma_buf_mapping_map() and dma_buf_mapping_unmap() so
importers supporting multiple mapping types can let the exporter select the
mapping type at map time and receive a type-tagged struct dma_buf_mapping. It
also wires up the SGT mapping type to these new operations via struct
dma_buf_sgt_mapping and dma_buf_sgt_mapping_table().

> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index dbcd83b6dcff7..7a5a379a372e7 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c
> @@ -366,6 +366,79 @@ int dma_buf_mapping_select(struct dma_buf_attachment *attach)
>  	return 0;
>  }
>  
> +/**
> + * dma_buf_mapping_map - Map an attachment with whichever type the exporter
> + *                       selects
> + * @attach: the attachment
> + * @dir: DMA direction
> + *
> + * For importers that listed more than one mapping type. The exporter picks the
> + * type from the possible matches and the result's &dma_buf_mapping.type records
> + * which was used. Must be called with the dma_resv lock held, and the lock must
> + * be held until the result has been acted on. Importers that listed a single
> + * type keep using that type's map function.
> + *
> + * Returns: a &struct dma_buf_mapping to pass to dma_buf_mapping_unmap(), or
> + * an ERR_PTR.
> + */
> +struct dma_buf_mapping *dma_buf_mapping_map(struct dma_buf_attachment *attach,
> +					    enum dma_data_direction dir)
> +{
> +	struct dma_buf_mapping *mapping;
> +	int ret;
> +
> +	might_sleep();
> +
> +	if (WARN_ON(!attach || !attach->dmabuf))
> +		return ERR_PTR(-EINVAL);
> +
> +	dma_resv_assert_held(attach->dmabuf->resv);
> +
> +	ret = dma_buf_mapping_select(attach);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (!attach->map_type.type->map) {
> +		ret = -EOPNOTSUPP;
> +		goto err_release;
> +	}
> +
> +	mapping = attach->map_type.type->map(attach, dir);
> +	if (IS_ERR(mapping)) {
> +		ret = PTR_ERR(mapping);
> +		goto err_release;
> +	}
> +
> +	return mapping;
> +
> +err_release:
> +	/* Nothing was established, so the choice may be made afresh */
> +	attach->selected = false;
> +	return ERR_PTR(ret);
> +}

[Severity: Medium]
Can jumping to err_release in dma_buf_mapping_map() corrupt an already
established selection state on attach?

If attach->selected is already true when dma_buf_mapping_map() is entered
(for instance, after dma_buf_pin(), after a prior map/unmap cycle without an
intervening dma_buf_invalidate_mappings(), or while another mapping on the
same attachment is still active), dma_buf_mapping_select() returns 0 without
making a new choice:

drivers/dma-buf/dma-buf-mapping.c:dma_buf_mapping_select() {
	if (attach->selected)
		return 0;
	...
	attach->map_type = *match;
	attach->selected = true;
	return 0;
}

If the subsequent map operation fails (such as kzalloc_obj() returning NULL
in dma_buf_sgt_type_map(), or sgt_exp_ops->map_dma_buf() failing in
__dma_buf_sgt_map_attachment()), err_release unconditionally sets
attach->selected = false.

A later call to dma_buf_mapping_select() can then invoke
dmabuf->ops->select_mapping(attach) again and overwrite attach->map_type
while the buffer is still pinned or has an active mapping. If an earlier SGT
mapping is still active and attach->map_type is changed to a non-SGT type,
unmapping that earlier mapping via:

dma_buf_mapping_unmap()
  -> dma_buf_sgt_type_unmap()
    -> dma_buf_unmap_attachment()

will find dma_buf_get_sgt_ops(attach) == NULL, trigger WARN_ON(!sgt_exp_ops),
and return early without unmapping or freeing mapping->sgt.

Additionally, when attach->selected was false on entry and
dma_buf_mapping_select() picked a match other than attach->matches[0],
clearing attach->selected on error leaves attach->map_type overwritten rather
than restored to attach->matches[0].

Should dma_buf_mapping_map() remember whether attach->selected was already
true before calling dma_buf_mapping_select(), and only reset attach->selected
(and restore attach->map_type) if this call actually performed the selection?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026053623.202642-1-apopple@nvidia.com?part=7