From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: iommufd: take dma_resv lock before dma_buf_unpin() in release path Date: Wed, 27 May 2026 14:56:39 +1000 Message-ID: In-Reply-To: <20260526111034.4079-1-Ankit.Soni@amd.com> References: <20260526111034.4079-1-Ankit.Soni@amd.com> <20260526111034.4079-1-Ankit.Soni@amd.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good.** The patch adds the missing `dma_resv_lock()`/`dma_resv_unlock()` bracket ar= ound `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** =E2=80=94 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** =E2= =80=94 it acquires `dma_resv_lock` internally (`dma-buf.c:1078`), so holdin= g it around `dma_buf_detach()` would deadlock since `dma_resv_lock` is not = recursive. 3. **Symmetry with `iopt_map_dmabuf()`** =E2=80=94 the pin path at `pages.c= :1495-1513` holds `dma_resv_lock` around `dma_buf_pin()` and releases it be= fore `dma_buf_detach()` in the error path (`pages.c:1519-1523`). This patch= mirrors that exact pattern. 4. **`dma_resv_lock()` return value** =E2=80=94 `dma_resv_lock(resv, NULL)`= with a NULL context cannot return `-EDEADLK` (that only occurs with ww_acq= uire_ctx-based contention management). It can only block or succeed, so ign= oring the return value is acceptable here. This is consistent with how `iop= t_map_dmabuf()` and `dma_buf_detach()` both call it. 5. **No ordering concern with `pages->mutex`** =E2=80=94 the function is ca= lled 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 =E2=80=94 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 =E2=80=94 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