[PATCH] dma-buf: Move dma_resv_assert_held() after input validation

[PATCH] dma-buf: Move dma_resv_assert_held() after input validation

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

Move dma_resv_assert_held() after the validation of 'attach' and
'attach->dmabuf' to avoid a potential null pointer dereference if
the function is ever called with invalid arguments.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/dma-buf/dma-buf-mapping.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
index 794acff..e6ecd6c 100644
--- a/drivers/dma-buf/dma-buf-mapping.c
+++ b/drivers/dma-buf/dma-buf-mapping.c
@@ -102,12 +102,12 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
 	size_t i;
 	int ret;
 
-	dma_resv_assert_held(attach->dmabuf->resv);
-
 	if (WARN_ON(!attach || !attach->dmabuf || !provider))
 		/* This function is supposed to work on MMIO memory only */
 		return ERR_PTR(-EINVAL);
 
+	dma_resv_assert_held(attach->dmabuf->resv);
+
 	dma = kzalloc_obj(*dma);
 	if (!dma)
 		return ERR_PTR(-ENOMEM);
-- 
2.9.4

Claude review: dma-buf: Move dma_resv_assert_held() after input validation

[Claude Code Review Bot]

Overall Series Review

Subject: dma-buf: Move dma_resv_assert_held() after input validation
Author: lirongqing <lirongqing@baidu.com>
Patches: 1
Reviewed: 2026-05-28T12:25:30.997300

---

This is a single patch that reorders a `dma_resv_assert_held()` call to come after NULL-pointer validation of `attach` and `attach->dmabuf` in `dma_buf_phys_vec_to_sgt()`.

**The patch is technically correct in what it does** — it does move the `dma_resv_assert_held()` after the WARN_ON check — but it addresses a problem that effectively cannot happen in practice.

**Verdict: Low-value patch, borderline NAK-worthy.**

---
Generated by Claude Code Patch Reviewer

Claude review: dma-buf: Move dma_resv_assert_held() after input validation

[Claude Code Review Bot]

Patch Review

**The change:**
```c
-	dma_resv_assert_held(attach->dmabuf->resv);
-
 	if (WARN_ON(!attach || !attach->dmabuf || !provider))
 		/* This function is supposed to work on MMIO memory only */
 		return ERR_PTR(-EINVAL);

+	dma_resv_assert_held(attach->dmabuf->resv);
```

**Issues:**

1. **The "bug" is hypothetical and cannot trigger in practice.** The commit message says "avoid a potential null pointer dereference if the function is ever called with invalid arguments," but examining all callers shows this cannot happen. The function is called from `vfio_pci_dma_buf_map()` and `uverbs_std_types_dmabuf.c`, both of which are dma-buf `map_dma_buf` callbacks. These callbacks receive a `struct dma_buf_attachment *` from the dma-buf core, which guarantees `attachment` and `attachment->dmabuf` are valid — the core would have already dereferenced them to acquire the reservation lock before calling the callback. In fact, the vfio caller already dereferences `attachment->dmabuf->priv` on line 54 *before* calling this function, so if `attach` or `attach->dmabuf` were NULL, the system would have already crashed before reaching `dma_buf_phys_vec_to_sgt()`.

2. **The WARN_ON itself is arguably wrong.** The `WARN_ON(!attach || !attach->dmabuf)` check is defensive against conditions that the dma-buf framework structurally prevents. Moving the assert after it doesn't fix any real problem — it just rearranges code around a condition that can never be true. Note that `dma_resv_assert_held()` expands to `lockdep_assert_held()`, which is a no-op in non-lockdep (i.e., production) builds, so even the "null deref" would only trigger in debug/lockdep-enabled kernels where developers are actively testing.

3. **No functional change in production.** Since `dma_resv_assert_held()` compiles to nothing without `CONFIG_LOCKDEP`, this patch has zero effect on release kernels. The only scenario where ordering matters is a lockdep-enabled debug kernel being called with a NULL attachment — a scenario that cannot occur through any existing code path.

**Summary:** The patch is not wrong, but it fixes a non-existent problem. The WARN_ON guards in this function are themselves questionable (they defend against structurally impossible NULL pointers from the dma-buf core), and reordering code around them doesn't add meaningful value. If the intent is defensive hardening, a better approach would be to either remove the unnecessary WARN_ON checks entirely or leave the code as-is, since both the assertion and the validation are guarding against conditions that cannot occur.

---
Generated by Claude Code Patch Reviewer