From 3d5af9d247cfd6c581e3a9d8e93a398cba6a8f61 Mon Sep 17 00:00:00 2001
From: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Thu, 14 May 2020 22:11:17 +0200
Subject: drm/msm: Don't call dma_buf_vunmap without _vmap
Git-commit: 8b6b7d84bfce9a2a29e66147d7acafd0be9bb211
Patch-mainline: v5.9-rc1
References: jsc#SLE-12680, jsc#SLE-12880, jsc#SLE-12882, jsc#SLE-12883, jsc#SLE-13496, jsc#SLE-15322
I honestly don't exactly understand what's going on here, but the
current code is wrong for sure: It calls dma_buf_vunmap without ever
calling dma_buf_vmap.
What I'm not sure about is whether the WARN_ON is correct:
- msm imports dma-buf using drm_prime_sg_to_page_addr_arrays. Which is
a pretty neat layering violation of how you shouldn't peek behind
the curtain of the dma-buf exporter, but par for course. Note that
all the nice new helpers don't (and we should probably have a bit a
warning about this in the kerneldoc).
- but then in the get_vaddr() in msm_gem.c, we seems to happily wrap a
vmap() around any object with ->pages set (so including imported
dma-buf).
- I'm not seeing any guarantees that userspace can't use an imported
dma-buf for e.g. MSM_SUBMIT_CMD_BUF in a5xx_submit_in_rb, so no
guarantees that an imported dma-buf won't end up with a ->vaddr set.
But even if that WARN_ON is wrong, cleaning up a vmap() done by msm by
calling dma_buf_vunmap is the wrong thing to do.
v2: Rob said in review that we do indeed have a gap in get_vaddr() that
needs to be plugged. But the users I've found aren't legit users on
imported dma-buf, so we can just reject that.
Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Link: https://patchwork.freedesktop.org/patch/msgid/20200514201117.465146-1-daniel.vetter@ffwll.ch
Signed-off-by: Patrik Jakobsson <pjakobsson@suse.de>
---
drivers/gpu/drm/msm/msm_gem.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index f01162257e71..38b0c0e1f83e 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -571,6 +571,9 @@ static void *get_vaddr(struct drm_gem_object *obj, unsigned madv)
struct msm_gem_object *msm_obj = to_msm_bo(obj);
int ret = 0;
+ if (obj->import_attach)
+ return ERR_PTR(-ENODEV);
+
mutex_lock(&msm_obj->lock);
if (WARN_ON(msm_obj->madv > madv)) {
@@ -924,8 +927,7 @@ static void free_object(struct msm_gem_object *msm_obj)
put_iova(obj);
if (obj->import_attach) {
- if (msm_obj->vaddr)
- dma_buf_vunmap(obj->import_attach->dmabuf, msm_obj->vaddr);
+ WARN_ON(msm_obj->vaddr);
/* Don't drop the pages for imported dmabuf, as they are not
* ours, just free the array we allocated:
--
2.29.2