| Message ID | 20220531200857.136547-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm: Ensure mmap offset is initialized | expand |
On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > If a GEM object is allocated, and then exported as a dma-buf fd which is > mmap'd before or without the GEM buffer being directly mmap'd, the > vma_node could be unitialized. This leads to a situation where the CPU > mapping is not correctly torn down in drm_vma_node_unmap(). > > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/msm/msm_drv.c | 2 +- > drivers/gpu/drm/msm/msm_drv.h | 1 + > drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++ > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 44485363f37a..14ab9a627d8b 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = { > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > - .gem_prime_mmap = drm_gem_prime_mmap, > + .gem_prime_mmap = msm_gem_prime_mmap, > #ifdef CONFIG_DEBUG_FS > .debugfs_init = msm_debugfs_init, > #endif > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index bb052071b16d..090b8074fec7 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t > void msm_gem_shrinker_init(struct drm_device *dev); > void msm_gem_shrinker_cleanup(struct drm_device *dev); > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj); > int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map); > void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map); > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c > index 94ab705e9b8a..dcc8a573bc76 100644 > --- a/drivers/gpu/drm/msm/msm_gem_prime.c > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c > @@ -11,6 +11,21 @@ > #include "msm_drv.h" > #include "msm_gem.h" > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > +{ > + int ret; > + > + /* Ensure the mmap offset is initialized. We lazily initialize it, > + * so if it has not been first mmap'd directly as a GEM object, the > + * mmap offset will not be already initialized. > + */ > + ret = drm_gem_create_mmap_offset(obj); > + if (ret) > + return ret; Wouldn't it be better to have this call directly in the drm_gem_prime_mmap() ? This way all drivers can be lazy. > + > + return drm_gem_prime_mmap(obj, vma); > +} > + > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj) > { > struct msm_gem_object *msm_obj = to_msm_bo(obj); > -- > 2.36.1 >
On Tue, May 31, 2022 at 1:34 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote: > > > > From: Rob Clark <robdclark@chromium.org> > > > > If a GEM object is allocated, and then exported as a dma-buf fd which is > > mmap'd before or without the GEM buffer being directly mmap'd, the > > vma_node could be unitialized. This leads to a situation where the CPU > > mapping is not correctly torn down in drm_vma_node_unmap(). > > > > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset") > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > drivers/gpu/drm/msm/msm_drv.c | 2 +- > > drivers/gpu/drm/msm/msm_drv.h | 1 + > > drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++ > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index 44485363f37a..14ab9a627d8b 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = { > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > > - .gem_prime_mmap = drm_gem_prime_mmap, > > + .gem_prime_mmap = msm_gem_prime_mmap, > > #ifdef CONFIG_DEBUG_FS > > .debugfs_init = msm_debugfs_init, > > #endif > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > > index bb052071b16d..090b8074fec7 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.h > > +++ b/drivers/gpu/drm/msm/msm_drv.h > > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t > > void msm_gem_shrinker_init(struct drm_device *dev); > > void msm_gem_shrinker_cleanup(struct drm_device *dev); > > > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj); > > int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map); > > void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map); > > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c > > index 94ab705e9b8a..dcc8a573bc76 100644 > > --- a/drivers/gpu/drm/msm/msm_gem_prime.c > > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c > > @@ -11,6 +11,21 @@ > > #include "msm_drv.h" > > #include "msm_gem.h" > > > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > +{ > > + int ret; > > + > > + /* Ensure the mmap offset is initialized. We lazily initialize it, > > + * so if it has not been first mmap'd directly as a GEM object, the > > + * mmap offset will not be already initialized. > > + */ > > + ret = drm_gem_create_mmap_offset(obj); > > + if (ret) > > + return ret; > > Wouldn't it be better to have this call directly in the > drm_gem_prime_mmap() ? This way all drivers can be lazy. > yes.. that was my first[1] proposal. But there are differences of opinion, and in the mean time I want to get this particular issue fixed ;-) BR, -R [1] https://patchwork.freedesktop.org/patch/487597/ > > > + > > + return drm_gem_prime_mmap(obj, vma); > > +} > > + > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj) > > { > > struct msm_gem_object *msm_obj = to_msm_bo(obj); > > -- > > 2.36.1 > > > > > -- > With best wishes > Dmitry
On Tue, 31 May 2022 at 23:37, Rob Clark <robdclark@gmail.com> wrote: > > On Tue, May 31, 2022 at 1:34 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote: > > > > > > From: Rob Clark <robdclark@chromium.org> > > > > > > If a GEM object is allocated, and then exported as a dma-buf fd which is > > > mmap'd before or without the GEM buffer being directly mmap'd, the > > > vma_node could be unitialized. This leads to a situation where the CPU > > > mapping is not correctly torn down in drm_vma_node_unmap(). > > > > > > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset") > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > > --- > > > drivers/gpu/drm/msm/msm_drv.c | 2 +- > > > drivers/gpu/drm/msm/msm_drv.h | 1 + > > > drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++ > > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > > index 44485363f37a..14ab9a627d8b 100644 > > > --- a/drivers/gpu/drm/msm/msm_drv.c > > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = { > > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > > > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > > > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > > > - .gem_prime_mmap = drm_gem_prime_mmap, > > > + .gem_prime_mmap = msm_gem_prime_mmap, > > > #ifdef CONFIG_DEBUG_FS > > > .debugfs_init = msm_debugfs_init, > > > #endif > > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > > > index bb052071b16d..090b8074fec7 100644 > > > --- a/drivers/gpu/drm/msm/msm_drv.h > > > +++ b/drivers/gpu/drm/msm/msm_drv.h > > > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t > > > void msm_gem_shrinker_init(struct drm_device *dev); > > > void msm_gem_shrinker_cleanup(struct drm_device *dev); > > > > > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); > > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj); > > > int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map); > > > void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map); > > > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c > > > index 94ab705e9b8a..dcc8a573bc76 100644 > > > --- a/drivers/gpu/drm/msm/msm_gem_prime.c > > > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c > > > @@ -11,6 +11,21 @@ > > > #include "msm_drv.h" > > > #include "msm_gem.h" > > > > > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > > +{ > > > + int ret; > > > + > > > + /* Ensure the mmap offset is initialized. We lazily initialize it, > > > + * so if it has not been first mmap'd directly as a GEM object, the > > > + * mmap offset will not be already initialized. > > > + */ > > > + ret = drm_gem_create_mmap_offset(obj); > > > + if (ret) > > > + return ret; > > > > Wouldn't it be better to have this call directly in the > > drm_gem_prime_mmap() ? This way all drivers can be lazy. > > > > yes.. that was my first[1] proposal. But there are differences of > opinion, and in the mean time I want to get this particular issue > fixed ;-) Ack, excuse me, I probably skipped the thread. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > BR, > -R > > [1] https://patchwork.freedesktop.org/patch/487597/ > > > > > > + > > > + return drm_gem_prime_mmap(obj, vma); > > > +} > > > + > > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj) > > > { > > > struct msm_gem_object *msm_obj = to_msm_bo(obj); > > > -- > > > 2.36.1 > > > > > > > > > -- > > With best wishes > > Dmitry
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 44485363f37a..14ab9a627d8b 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = { .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, - .gem_prime_mmap = drm_gem_prime_mmap, + .gem_prime_mmap = msm_gem_prime_mmap, #ifdef CONFIG_DEBUG_FS .debugfs_init = msm_debugfs_init, #endif diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index bb052071b16d..090b8074fec7 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t void msm_gem_shrinker_init(struct drm_device *dev); void msm_gem_shrinker_cleanup(struct drm_device *dev); +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj); int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map); void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map); diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c index 94ab705e9b8a..dcc8a573bc76 100644 --- a/drivers/gpu/drm/msm/msm_gem_prime.c +++ b/drivers/gpu/drm/msm/msm_gem_prime.c @@ -11,6 +11,21 @@ #include "msm_drv.h" #include "msm_gem.h" +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) +{ + int ret; + + /* Ensure the mmap offset is initialized. We lazily initialize it, + * so if it has not been first mmap'd directly as a GEM object, the + * mmap offset will not be already initialized. + */ + ret = drm_gem_create_mmap_offset(obj); + if (ret) + return ret; + + return drm_gem_prime_mmap(obj, vma); +} + struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj) { struct msm_gem_object *msm_obj = to_msm_bo(obj);