| Message ID | 20200514201117.465146-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8b6b7d84bfce9a2a29e66147d7acafd0be9bb211 |
| Headers | show |
| Series | drm/msm: Don't call dma_buf_vunmap without _vmap | expand |
On Thu, May 14, 2020 at 10:11:17PM +0200, Daniel Vetter wrote: > 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. > > 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 Ping for some review/ack so I can start landing thist stuff please? Thanks, Daniel > --- > 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 5a6a79fbc9d6..e70abd1cde43 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c > @@ -554,6 +554,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)) { > @@ -907,8 +910,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.26.2 >
On Thu, May 14, 2020 at 1:11 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > 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. > > 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 Reviewed-by: Rob Clark <robdclark@gmail.com> > --- > 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 5a6a79fbc9d6..e70abd1cde43 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c > @@ -554,6 +554,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)) { > @@ -907,8 +910,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.26.2 >
On Sun, May 31, 2020 at 09:02:11AM -0700, Rob Clark wrote: > On Thu, May 14, 2020 at 1:11 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > 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. > > > > 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 > > Reviewed-by: Rob Clark <robdclark@gmail.com> Queued in drm-misc-next for 5.9, thanks for your review. -Daniel > > > --- > > 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 5a6a79fbc9d6..e70abd1cde43 100644 > > --- a/drivers/gpu/drm/msm/msm_gem.c > > +++ b/drivers/gpu/drm/msm/msm_gem.c > > @@ -554,6 +554,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)) { > > @@ -907,8 +910,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.26.2 > >
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index 5a6a79fbc9d6..e70abd1cde43 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -554,6 +554,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)) { @@ -907,8 +910,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:
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. 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 --- drivers/gpu/drm/msm/msm_gem.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)