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