[bug report] dma-buf: Move dma_buf_attach() to dynamic locking specification

[bug report] dma-buf: Move dma_buf_attach() to dynamic locking specification

[Dan Carpenter]

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.

  1002  
  1003          if (attach->sgt) {
  1004  
  1005                  __unmap_dma_buf(attach, attach->sgt, attach->dir);
  1006  
  1007                  if (dma_buf_is_dynamic(attach->dmabuf))
  1008                          dmabuf->ops->unpin(attach);
  1009          }
  1010          list_del(&attach->node);
  1011  
  1012          dma_resv_unlock(dmabuf->resv);
  1013  
  1014          if (dmabuf->ops->detach)
  1015                  dmabuf->ops->detach(dmabuf, attach);
  1016  
  1017          kfree(attach);
  1018  }

regards,
dan carpenter

Re: [bug report] dma-buf: Move dma_buf_attach() to dynamic locking specification

[Dmitry Osipenko]

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