On 10/25/22 14:41, Dan Carpenter wrote:
> Hello Dmitry Osipenko,
>
> The patch 809d9c72c2f8: "dma-buf: Move dma_buf_attach() to dynamic
> locking specification" from Oct 17, 2022, leads to the following
> Smatch static checker warning:
>
> drivers/dma-buf/dma-buf.c:957 dma_buf_dynamic_attach()
> error: double unlocked 'dmabuf->resv' (orig line 915)
>
> drivers/dma-buf/dma-buf.c
> 987 /**
> 988 * dma_buf_detach - Remove the given attachment from dmabuf's attachments list
> 989 * @dmabuf: [in] buffer to detach from.
> 990 * @attach: [in] attachment to be detached; is free'd after this call.
> 991 *
> 992 * Clean up a device attachment obtained by calling dma_buf_attach().
> 993 *
> 994 * Optionally this calls &dma_buf_ops.detach for device-specific detach.
> 995 */
> 996 void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
> 997 {
> 998 if (WARN_ON(!dmabuf || !attach))
> 999 return;
> 1000
> 1001 dma_resv_lock(attach->dmabuf->resv, NULL);
>
> In the original code used to take this both the "attach->dmabuf->resv"
> and "dmabuf->resv" locks and unlock them both. But now it takes one
> lock and unlocks the other. Seems sus.
It will be the same lock. Apparently I copied the part of code from
other function, that's why lock/unlock aren't consistent there. The
dma_buf_detach() doesn't really need the `dmabuf` argument, perhaps it
was more useful in the past.
I'll prepare the patch to clean up the locking pointer. Thank you for
the report!
--
Best regards,
Dmitry
