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 <[email protected]> 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/[email protected]?part=7
