| Message ID | 20221017172229.42269-19-dmitry.osipenko@collabora.com |
|---|---|
| State | Accepted |
| Commit | d078fd9b8daa282a0c713daa433315940bbf8188 |
| Headers | show |
| Series | None | expand |
On Mon, 17 Oct 2022 at 19:25, Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > Move dma_buf_mmap() function to the dynamic locking specification by > taking the reservation lock. Neither of the today's drivers take the > reservation lock within the mmap() callback, hence it's safe to enforce > the locking. > > Acked-by: Sumit Semwal <sumit.semwal@linaro.org> > Acked-by: Christian König <christian.koenig@amd.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Just noticed this while reading code ... this patch seems to have missed dma_buf_mmap_internal()? Might be good if at least some drivers gain a dma_resv_assert_held in that path to make sure we're not quite this bad, together with fixing this issue. -Daniel > --- > drivers/dma-buf/dma-buf.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index f54c649f922a..f149b384f4dd 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1390,6 +1390,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF); > int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, > unsigned long pgoff) > { > + int ret; > + > if (WARN_ON(!dmabuf || !vma)) > return -EINVAL; > > @@ -1410,7 +1412,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, > vma_set_file(vma, dmabuf->file); > vma->vm_pgoff = pgoff; > > - return dmabuf->ops->mmap(dmabuf, vma); > + dma_resv_lock(dmabuf->resv, NULL); > + ret = dmabuf->ops->mmap(dmabuf, vma); > + dma_resv_unlock(dmabuf->resv); > + > + return ret; > } > EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF); > > -- > 2.37.3 >
On 11/7/22 20:25, Daniel Vetter wrote: >> Move dma_buf_mmap() function to the dynamic locking specification by >> taking the reservation lock. Neither of the today's drivers take the >> reservation lock within the mmap() callback, hence it's safe to enforce >> the locking. >> >> Acked-by: Sumit Semwal <sumit.semwal@linaro.org> >> Acked-by: Christian König <christian.koenig@amd.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Just noticed this while reading code ... this patch seems to have > missed dma_buf_mmap_internal()? > > Might be good if at least some drivers gain a dma_resv_assert_held in > that path to make sure we're not quite this bad, together with fixing > this issue. Good catch! I'll prepare the patches.
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f54c649f922a..f149b384f4dd 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1390,6 +1390,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF); int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, unsigned long pgoff) { + int ret; + if (WARN_ON(!dmabuf || !vma)) return -EINVAL; @@ -1410,7 +1412,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, vma_set_file(vma, dmabuf->file); vma->vm_pgoff = pgoff; - return dmabuf->ops->mmap(dmabuf, vma); + dma_resv_lock(dmabuf->resv, NULL); + ret = dmabuf->ops->mmap(dmabuf, vma); + dma_resv_unlock(dmabuf->resv); + + return ret; } EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);