Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] dma-buf: dma_buf_get_sgt_ops() called before !attach NULL check --
commit b7db1c5ac9aaaa1115dc4420ef21a9d03108a96b Author: Jason Gunthorpe <[email protected]> dma-buf: Route SGT related actions through attach->map_type This commit routes sg_table map, unmap, and debugfs dump operations through attach->map_type instead of calling attach->dmabuf->ops directly. > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 16a7fbb8aaabe..348b47f266b48 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1253,12 +1253,14 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_unpin, "DMA_BUF"); > struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > enum dma_data_direction direction) > { > + const struct dma_buf_mapping_sgt_exp_ops *sgt_exp_ops = > + dma_buf_get_sgt_ops(attach); > struct sg_table *sg_table; > signed long ret; > > might_sleep(); > > - if (WARN_ON(!attach || !attach->dmabuf)) > + if (WARN_ON(!attach || !attach->dmabuf || !sgt_exp_ops)) > return ERR_PTR(-EINVAL); [ ... ] > @@ -1366,15 +1368,18 @@ void dma_buf_unmap_attachment(struct > dma_buf_attachment *attach, > struct sg_table *sg_table, > enum dma_data_direction direction) > { > + const struct dma_buf_mapping_sgt_exp_ops *sgt_exp_ops = > + dma_buf_get_sgt_ops(attach); > + > might_sleep(); > > - if (WARN_ON(!attach || !attach->dmabuf || !sg_table)) > + if (WARN_ON(!attach || !attach->dmabuf || !sg_table || !sgt_exp_ops)) > return; [Severity: Medium] Can calling dma_buf_get_sgt_ops(attach) in the variable initializers of dma_buf_map_attachment() and dma_buf_unmap_attachment() cause a null pointer dereference when attach is null? In include/linux/dma-buf-mapping.h, dma_buf_get_sgt_ops() unconditionally dereferences attach: static inline const struct dma_buf_mapping_sgt_exp_ops * dma_buf_get_sgt_ops(struct dma_buf_attachment *attach) { if (attach->map_type.type != &dma_buf_mapping_sgt_type) return NULL; return container_of(attach->map_type.exp_ops, struct dma_buf_mapping_sgt_exp_ops, ops); } Because dma_buf_get_sgt_ops(attach) runs before the WARN_ON(!attach || ...) checks in both functions, passing a null attach will dereference a null pointer instead of triggering the warning and returning early, and the compiler may also optimize away the subsequent !attach checks. Note that while the dma_buf_map_attachment() path is refactored in a later commit in this series ("dma-buf: Let the exporter select mapping type at pin/map") when __dma_buf_sgt_map_attachment() is split out after the WARN_ON(!attach || !attach->dmabuf) check, dma_buf_unmap_attachment() (later renamed to dma_buf_sgt_unmap_attachment() in "treewide: Rename dma_buf_unmap_attachment(_unlocked) to dma_buf_sgt_*") keeps the dma_buf_get_sgt_ops(attach) call before WARN_ON(!attach || ...). Should sgt_exp_ops be initialized after the !attach check in both functions? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
