Re: [PATCH 05/26] dma-buf: Let the exporter select mapping type at pin/map

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dma-buf: dma_buf_map_attachment() clears preexisting attach->selected on error
- [Medium] dma-buf: dma_buf_pin() leaves attach->selected true when ops->pin() fails
--

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

dma-buf: Let the exporter select mapping type at pin/map

This commit adds dma_buf_ops.select_mapping() so the exporter can select a
mapping type from attach->matches when an attachment is first pinned or
mapped, recording the choice in attach->selected until mappings are
invalidated via dma_buf_invalidate_mappings().

> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 348b47f266b48..03f06c4b957ca 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1201,6 +1201,10 @@ int dma_buf_pin(struct dma_buf_attachment *attach)
>  
>  	dma_resv_assert_held(dmabuf->resv);
>  
> +	ret = dma_buf_mapping_select(attach);
> +	if (ret)
> +		return ret;
> +
>  	if (dmabuf->ops->pin)
>  		ret = dmabuf->ops->pin(attach);
>  

[Severity: Medium]
Should dma_buf_pin() reset attach->selected to false if dmabuf->ops->pin()
fails when attach->selected was false upon entry?

When dma_buf_pin() is called on an unselected attachment,
dma_buf_mapping_select() sets attach->map_type and attach->selected = true:

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

If dmabuf->ops->pin(attach) subsequently returns an error, dma_buf_pin()
returns that error while leaving attach->selected set to true even though the
pin was not established. Because the attachment is not pinned, the exporter
may migrate the buffer elsewhere without invalidating it (and for attachments
without importer_ops->invalidate_mappings, dma_buf_invalidate_mappings()
skips clearing attach->selected). A later retry of dma_buf_pin() or call to
dma_buf_map_attachment() will see attach->selected == true in
dma_buf_mapping_select() and skip dmabuf->ops->select_mapping(), leaving a
stale attach->map_type in place.

[ ... ]

> @@ -1324,6 +1305,52 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  
>  	return sg_table;
>  }
> +

[ ... ]

> +struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
> +					enum dma_data_direction direction)
> +{
> +	struct sg_table *sg_table;
> +	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);
> +
> +	sg_table = __dma_buf_sgt_map_attachment(attach, direction);
> +	if (IS_ERR(sg_table))
> +		/* Nothing was established, so the choice may be made afresh */
> +		attach->selected = false;

[Severity: High]
What happens in dma_buf_map_attachment() if attach->selected was already true
prior to calling dma_buf_mapping_select() and __dma_buf_sgt_map_attachment()
fails?

An importer can enter dma_buf_map_attachment() with attach->selected == true
because the attachment was previously pinned via dma_buf_pin(), because it is
remapping after a prior map/unmap cycle without an intervening
dma_buf_invalidate_mappings(), or because another mapping on the same
attachment is already active. In that case, dma_buf_mapping_select() returns 0
immediately without making a new selection:

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

If __dma_buf_sgt_map_attachment() then fails (for example, sgt_exp_ops is
NULL, map_dma_buf() or dma_buf_wrap_sg_table() returns -ENOMEM, or
dma_resv_wait_timeout() is interrupted), dma_buf_map_attachment()
unconditionally clears attach->selected = false.

A subsequent dma_buf_map_attachment() or dma_buf_pin() call will then invoke
dmabuf->ops->select_mapping(attach) again without
dma_buf_invalidate_mappings() having been delivered, which can overwrite
attach->map_type while the attachment is still pinned or while an earlier
mapping is still active. When that earlier mapping is later unmapped in
dma_buf_unmap_attachment():

drivers/dma-buf/dma-buf.c:dma_buf_unmap_attachment() {
	const struct dma_buf_mapping_sgt_exp_ops *sgt_exp_ops =
		dma_buf_get_sgt_ops(attach);
	...
	if (WARN_ON(!attach || !attach->dmabuf || !sg_table || !sgt_exp_ops))
		return;
	...
	dma_buf_unwrap_sg_table(&sg_table);
	sgt_exp_ops->unmap_dma_buf(attach, sg_table, direction);
}

 wouldn't dma_buf_get_sgt_ops(attach) read the overwritten attach->map_type
and either call the wrong unmap_dma_buf() callback or hit
WARN_ON(!sgt_exp_ops) and leak the mapping?

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