| Message ID | 20200511093554.211493-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/9] drm/msm: Don't call dma_buf_vunmap without _vmap | expand |
On Mon, May 11, 2020 at 2:36 AM 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, and that 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. fwiw, a5xx_submit_in_rb() isn't a "normal" path (build-time disabled by default, and restricted to sudo).. it really only exists to simplify poking at fw. There could be vmap's in the msm_gem_submit path, however. If we don't, we should probably just disallow using an imported dma-buf as cmdstream.. I don't think there is any sane reason to permit that. We should probably also disallow get_vaddr() on imported buffers. BR, -R > > But even if that WARN_ON is wrong, cleaning up a vmap() done by msm by > calling dma_buf_vmap is the wrong thing to do. > > 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 | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > index 5a6a79fbc9d6..3305a457960e 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c > @@ -907,8 +907,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 Mon, May 11, 2020 at 5:24 PM Rob Clark <robdclark@gmail.com> wrote: > > On Mon, May 11, 2020 at 2:36 AM 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, and that 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. > > fwiw, a5xx_submit_in_rb() isn't a "normal" path (build-time disabled > by default, and restricted to sudo).. it really only exists to > simplify poking at fw. > > There could be vmap's in the msm_gem_submit path, however. If we > don't, we should probably just disallow using an imported dma-buf as > cmdstream.. I don't think there is any sane reason to permit that. We > should probably also disallow get_vaddr() on imported buffers. Yeah if that's possible and won't blow up (I can't test) I think it'd be best. Something like if (bo->import_attach) return NULL; should do the trick I think. Should I type that up as v2 of this? -Daniel > > BR, > -R > > > > > But even if that WARN_ON is wrong, cleaning up a vmap() done by msm by > > calling dma_buf_vmap is the wrong thing to do. > > > > 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 | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > > index 5a6a79fbc9d6..3305a457960e 100644 > > --- a/drivers/gpu/drm/msm/msm_gem.c > > +++ b/drivers/gpu/drm/msm/msm_gem.c > > @@ -907,8 +907,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 Mon, May 11, 2020 at 8:29 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Mon, May 11, 2020 at 5:24 PM Rob Clark <robdclark@gmail.com> wrote: > > > > On Mon, May 11, 2020 at 2:36 AM 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, and that 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. > > > > fwiw, a5xx_submit_in_rb() isn't a "normal" path (build-time disabled > > by default, and restricted to sudo).. it really only exists to > > simplify poking at fw. > > > > There could be vmap's in the msm_gem_submit path, however. If we > > don't, we should probably just disallow using an imported dma-buf as > > cmdstream.. I don't think there is any sane reason to permit that. We > > should probably also disallow get_vaddr() on imported buffers. > > Yeah if that's possible and won't blow up (I can't test) I think it'd > be best. Something like > if (bo->import_attach) return NULL; should do the trick I think. > Should I type that up as v2 of this? Sure. It should probably be something like if (obj->import_attach) return ERR_PTR(-ESOMETHING) looks like the gem-submit path handles an IS_ERR() return BR, -R > -Daniel > > > > > BR, > > -R > > > > > > > > But even if that WARN_ON is wrong, cleaning up a vmap() done by msm by > > > calling dma_buf_vmap is the wrong thing to do. > > > > > > 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 | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > > > index 5a6a79fbc9d6..3305a457960e 100644 > > > --- a/drivers/gpu/drm/msm/msm_gem.c > > > +++ b/drivers/gpu/drm/msm/msm_gem.c > > > @@ -907,8 +907,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 > > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index 5a6a79fbc9d6..3305a457960e 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -907,8 +907,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, and that 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_vmap is the wrong thing to do. 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 | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)