[PATCH] iommufd: take dma_resv lock before dma_buf_unpin() in release path

[PATCH] iommufd: take dma_resv lock before dma_buf_unpin() in release path

[Ankit Soni]

dma_buf_unpin() requires the caller to hold the exporter's dma_resv
lock:

  void dma_buf_unpin(struct dma_buf_attachment *attach)
  {
          ...
          dma_resv_assert_held(dmabuf->resv);
          ...
  }

iopt_release_pages() calls dma_buf_unpin() without taking that lock,
so every iommufd_ioas_destroy()/iommufd_ioas_unmap() that releases
the last reference on a DMABUF-backed iopt_pages triggers a WARN.
This was hit while running tools/testing/selftests/iommu/iommufd:

  WARNING: drivers/dma-buf/dma-buf.c:1137 at dma_buf_unpin+0x62/0x70
  RIP: 0010:dma_buf_unpin+0x62/0x70
  Call Trace:
   <TASK>
   dma_buf_unpin+0x62/0x70
   iopt_release_pages+0xe4/0x190
   iopt_unmap_iova_range+0x1c7/0x290
   iopt_unmap_all+0x1a/0x30
   iommufd_ioas_destroy+0x1d/0x50
   iommufd_fops_release+0x93/0x150
   __fput+0xfc/0x2c0
   __x64_sys_close+0x3d/0x80
   do_syscall_64+0x65/0x180
   </TASK>

Take the dma_resv lock around dma_buf_unpin() in iopt_release_pages(),
matching the iopt_map_dmabuf() convention. dma_buf_detach() acquires the
reservation lock internally, so it must remain outside the locked region.

Fixes: 8c5f9645c389 ("iommufd: Add dma_buf_pin()")
Reported-by: Ankit Soni <Ankit.Soni@amd.com>
Signed-off-by: Ankit Soni <Ankit.Soni@amd.com>
---
 drivers/iommu/iommufd/pages.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index 9bdb2945afe1..7b64002e54b9 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -1663,7 +1663,9 @@ void iopt_release_pages(struct kref *kref)
 	if (iopt_is_dmabuf(pages) && pages->dmabuf.attach) {
 		struct dma_buf *dmabuf = pages->dmabuf.attach->dmabuf;
 
+		dma_resv_lock(dmabuf->resv, NULL);
 		dma_buf_unpin(pages->dmabuf.attach);
+		dma_resv_unlock(dmabuf->resv);
 		dma_buf_detach(dmabuf, pages->dmabuf.attach);
 		dma_buf_put(dmabuf);
 		WARN_ON(!list_empty(&pages->dmabuf.tracker));
-- 
2.43.0

Re: [PATCH] iommufd: take dma_resv lock before dma_buf_unpin() in release path

[Jason Gunthorpe]

On Tue, May 26, 2026 at 11:10:34AM +0000, Ankit Soni wrote:
> dma_buf_unpin() requires the caller to hold the exporter's dma_resv
> lock:
> 
>   void dma_buf_unpin(struct dma_buf_attachment *attach)
>   {
>           ...
>           dma_resv_assert_held(dmabuf->resv);
>           ...
>   }
> 
> iopt_release_pages() calls dma_buf_unpin() without taking that lock,
> so every iommufd_ioas_destroy()/iommufd_ioas_unmap() that releases
> the last reference on a DMABUF-backed iopt_pages triggers a WARN.
> This was hit while running tools/testing/selftests/iommu/iommufd:

Any idea why this is comming up now? Did I run the tests without some
kind of debug option to turn on that assertion maybe?

Jason

Re: [PATCH] iommufd: take dma_resv lock before dma_buf_unpin() in release path

[Ankit Soni]

On Tue, May 26, 2026 at 09:26:56AM -0300, Jason Gunthorpe wrote:
> On Tue, May 26, 2026 at 11:10:34AM +0000, Ankit Soni wrote:
> > dma_buf_unpin() requires the caller to hold the exporter's dma_resv
> > lock:
> > 
> >   void dma_buf_unpin(struct dma_buf_attachment *attach)
> >   {
> >           ...
> >           dma_resv_assert_held(dmabuf->resv);
> >           ...
> >   }
> > 
> > iopt_release_pages() calls dma_buf_unpin() without taking that lock,
> > so every iommufd_ioas_destroy()/iommufd_ioas_unmap() that releases
> > the last reference on a DMABUF-backed iopt_pages triggers a WARN.
> > This was hit while running tools/testing/selftests/iommu/iommufd:
> 
> Any idea why this is comming up now? Did I run the tests without some
> kind of debug option to turn on that assertion maybe?
> 
> Jason

The assertion is gated by CONFIG_LOCKDEP. My config has it on via
CONFIG_DEBUG_LOCK_ALLOC=y (LOCK_STAT, PROVE_LOCKING and
DEBUG_WW_MUTEX_SLOWPATH each select-chain to LOCKDEP as well).

-Ankit

Claude review: iommufd: take dma_resv lock before dma_buf_unpin() in release path

[Claude Code Review Bot]

Overall Series Review

Subject: iommufd: take dma_resv lock before dma_buf_unpin() in release path
Author: Ankit Soni <Ankit.Soni@amd.com>
Patches: 3
Reviewed: 2026-05-27T14:56:38.808531

---

This is a single-patch bug fix for a real locking deficiency in iommufd's dmabuf teardown path. The analysis is correct: `dma_buf_unpin()` requires the caller to hold `dma_resv_lock`, and `iopt_release_pages()` was calling it without that lock, triggering a `WARN` via `dma_resv_assert_held()`. The fix is minimal and mirrors the existing lock/unlock pattern used in `iopt_map_dmabuf()` (the pin path). The commit message is well-written with a clear stack trace, explanation of the problem, and rationale for lock placement.

**Verdict: Correct fix, ready to merge with minor observations below.**

---
Generated by Claude Code Patch Reviewer

Claude review: iommufd: take dma_resv lock before dma_buf_unpin() in release path

[Claude Code Review Bot]

Patch Review

**Correctness: Good.**

The patch adds the missing `dma_resv_lock()`/`dma_resv_unlock()` bracket around `dma_buf_unpin()`:

```c
+		dma_resv_lock(dmabuf->resv, NULL);
 		dma_buf_unpin(pages->dmabuf.attach);
+		dma_resv_unlock(dmabuf->resv);
 		dma_buf_detach(dmabuf, pages->dmabuf.attach);
```

This is correct for several reasons:

1. **`dma_buf_unpin()` requires the lock** — confirmed by `dma_resv_assert_held(dmabuf->resv)` at `drivers/dma-buf/dma-buf.c:1136`.

2. **`dma_buf_detach()` is correctly kept outside the locked region** — it acquires `dma_resv_lock` internally (`dma-buf.c:1078`), so holding it around `dma_buf_detach()` would deadlock since `dma_resv_lock` is not recursive.

3. **Symmetry with `iopt_map_dmabuf()`** — the pin path at `pages.c:1495-1513` holds `dma_resv_lock` around `dma_buf_pin()` and releases it before `dma_buf_detach()` in the error path (`pages.c:1519-1523`). This patch mirrors that exact pattern.

4. **`dma_resv_lock()` return value** — `dma_resv_lock(resv, NULL)` with a NULL context cannot return `-EDEADLK` (that only occurs with ww_acquire_ctx-based contention management). It can only block or succeed, so ignoring the return value is acceptable here. This is consistent with how `iopt_map_dmabuf()` and `dma_buf_detach()` both call it.

5. **No ordering concern with `pages->mutex`** — the function is called from `kref_put`, meaning the refcount has hit zero. No other code path can hold `pages->mutex` for this object simultaneously, so there's no lock ordering issue in practice. The lockdep annotation in `iopt_map_dmabuf()` (taking `pages->mutex` inside `dma_resv_lock`) is satisfied trivially since the mutex isn't held here.

**Minor observations (not blocking):**

- The `Fixes:` tag references `8c5f9645c389 ("iommufd: Add dma_buf_pin()")` which is appropriate — that's the commit that introduced the `dma_buf_unpin()` call in this path without the required locking.

- The patch has both `Reported-by` and `Signed-off-by` from the same author, which is fine — the author found and fixed the bug.

- A `Cc: stable@vger.kernel.org` tag could be appropriate given the `Fixes:` tag, but the maintainer can add that at merge time.

**No issues found. The fix is minimal, correct, and well-justified.**

---
Generated by Claude Code Patch Reviewer