Message ID | 20210115083938.21747-1-tzimmermann@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm: Set vm_ops to GEM object's values during mmap | expand |
Hi Thomas, On 15/01/2021 08:39, Thomas Zimmermann wrote: > The GEM mmap code relies on the GEM object's mmap callback to set the > VMA's vm_ops field. This is easily forgotten and lead to a memory leak s/lead/can lead/ > in the CMA helpers. Instead set the vm_ops field in the DRM core code > to the GEM object's value. Drivers with different needs can override > this in their mmap callback. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions") > Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> This also works here. - https://paste.ubuntu.com/p/S2Q586rgwT/ Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Eric Anholt <eric@anholt.net> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/drm_gem.c | 23 ++++++++++++----------- > drivers/gpu/drm/drm_prime.c | 4 ++++ > 2 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 34b2f111c01c..54d95621fcbb 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > > vma->vm_private_data = obj; > > + if (!obj->funcs->vm_ops) { > + ret = -EINVAL; > + goto err_drm_gem_object_put; > + } > + vma->vm_ops = obj->funcs->vm_ops; > + > if (obj->funcs->mmap) { > ret = obj->funcs->mmap(obj, vma); > - if (ret) { > - drm_gem_object_put(obj); > - return ret; > - } > + if (ret) > + goto err_drm_gem_object_put; > WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); > } else { > - if (obj->funcs->vm_ops) > - vma->vm_ops = obj->funcs->vm_ops; > - else { > - drm_gem_object_put(obj); > - return -EINVAL; > - } > - > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; > vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > } > > return 0; > + > +err_drm_gem_object_put: > + drm_gem_object_put(obj); > + return ret; > } > EXPORT_SYMBOL(drm_gem_mmap_obj); > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 683aa29ecd3b..0549ec190583 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > > if (obj->funcs && obj->funcs->mmap) { > + if (!obj->funcs->vm_ops) > + return -EINVAL; > + vma->vm_ops = obj->funcs->vm_ops; > + > ret = obj->funcs->mmap(obj, vma); > if (ret) > return ret; >
Hi Am 15.01.21 um 10:17 schrieb Kieran Bingham: > Hi Thomas, > > On 15/01/2021 08:39, Thomas Zimmermann wrote: >> The GEM mmap code relies on the GEM object's mmap callback to set the >> VMA's vm_ops field. This is easily forgotten and lead to a memory leak > > s/lead/can lead/ > >> in the CMA helpers. Instead set the vm_ops field in the DRM core code >> to the GEM object's value. Drivers with different needs can override >> this in their mmap callback. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions") >> Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > This also works here. > - https://paste.ubuntu.com/p/S2Q586rgwT/ > > Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Great! I have to send out another iteration of the patch, but I think the change is such that I can keep the Tested-by. Best regards Thomas > >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: David Airlie <airlied@linux.ie> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: Eric Anholt <eric@anholt.net> >> Cc: dri-devel@lists.freedesktop.org >> --- >> drivers/gpu/drm/drm_gem.c | 23 ++++++++++++----------- >> drivers/gpu/drm/drm_prime.c | 4 ++++ >> 2 files changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index 34b2f111c01c..54d95621fcbb 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, >> >> vma->vm_private_data = obj; >> >> + if (!obj->funcs->vm_ops) { >> + ret = -EINVAL; >> + goto err_drm_gem_object_put; >> + } >> + vma->vm_ops = obj->funcs->vm_ops; >> + >> if (obj->funcs->mmap) { >> ret = obj->funcs->mmap(obj, vma); >> - if (ret) { >> - drm_gem_object_put(obj); >> - return ret; >> - } >> + if (ret) >> + goto err_drm_gem_object_put; >> WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); >> } else { >> - if (obj->funcs->vm_ops) >> - vma->vm_ops = obj->funcs->vm_ops; >> - else { >> - drm_gem_object_put(obj); >> - return -EINVAL; >> - } >> - >> vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; >> vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); >> vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); >> } >> >> return 0; >> + >> +err_drm_gem_object_put: >> + drm_gem_object_put(obj); >> + return ret; >> } >> EXPORT_SYMBOL(drm_gem_mmap_obj); >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 683aa29ecd3b..0549ec190583 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) >> vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); >> >> if (obj->funcs && obj->funcs->mmap) { >> + if (!obj->funcs->vm_ops) >> + return -EINVAL; >> + vma->vm_ops = obj->funcs->vm_ops; >> + >> ret = obj->funcs->mmap(obj, vma); >> if (ret) >> return ret; >> > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On 15/01/2021 09:28, Thomas Zimmermann wrote: > Hi > > Am 15.01.21 um 10:17 schrieb Kieran Bingham: >> Hi Thomas, >> >> On 15/01/2021 08:39, Thomas Zimmermann wrote: >>> The GEM mmap code relies on the GEM object's mmap callback to set the >>> VMA's vm_ops field. This is easily forgotten and lead to a memory leak >> >> s/lead/can lead/ >> >>> in the CMA helpers. Instead set the vm_ops field in the DRM core code >>> to the GEM object's value. Drivers with different needs can override >>> this in their mmap callback. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA >>> object functions") >>> Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> >> This also works here. >> - https://paste.ubuntu.com/p/S2Q586rgwT/ >> >> Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Great! I have to send out another iteration of the patch, but I think > the change is such that I can keep the Tested-by. I have this test automated now, so I'll kick off a validation run on the new patch as well. -- Kieran > > Best regards > Thomas > >> >>> Cc: Maxime Ripard <mripard@kernel.org> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: David Airlie <airlied@linux.ie> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Eric Anholt <eric@anholt.net> >>> Cc: dri-devel@lists.freedesktop.org >>> --- >>> drivers/gpu/drm/drm_gem.c | 23 ++++++++++++----------- >>> drivers/gpu/drm/drm_prime.c | 4 ++++ >>> 2 files changed, 16 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>> index 34b2f111c01c..54d95621fcbb 100644 >>> --- a/drivers/gpu/drm/drm_gem.c >>> +++ b/drivers/gpu/drm/drm_gem.c >>> @@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object >>> *obj, unsigned long obj_size, >>> vma->vm_private_data = obj; >>> + if (!obj->funcs->vm_ops) { >>> + ret = -EINVAL; >>> + goto err_drm_gem_object_put; >>> + } >>> + vma->vm_ops = obj->funcs->vm_ops; >>> + >>> if (obj->funcs->mmap) { >>> ret = obj->funcs->mmap(obj, vma); >>> - if (ret) { >>> - drm_gem_object_put(obj); >>> - return ret; >>> - } >>> + if (ret) >>> + goto err_drm_gem_object_put; >>> WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); >>> } else { >>> - if (obj->funcs->vm_ops) >>> - vma->vm_ops = obj->funcs->vm_ops; >>> - else { >>> - drm_gem_object_put(obj); >>> - return -EINVAL; >>> - } >>> - >>> vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | >>> VM_DONTDUMP; >>> vma->vm_page_prot = >>> pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); >>> vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); >>> } >>> return 0; >>> + >>> +err_drm_gem_object_put: >>> + drm_gem_object_put(obj); >>> + return ret; >>> } >>> EXPORT_SYMBOL(drm_gem_mmap_obj); >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>> index 683aa29ecd3b..0549ec190583 100644 >>> --- a/drivers/gpu/drm/drm_prime.c >>> +++ b/drivers/gpu/drm/drm_prime.c >>> @@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object >>> *obj, struct vm_area_struct *vma) >>> vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); >>> if (obj->funcs && obj->funcs->mmap) { >>> + if (!obj->funcs->vm_ops) >>> + return -EINVAL; >>> + vma->vm_ops = obj->funcs->vm_ops; >>> + >>> ret = obj->funcs->mmap(obj, vma); >>> if (ret) >>> return ret; >>> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 34b2f111c01c..54d95621fcbb 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1069,27 +1069,28 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, vma->vm_private_data = obj; + if (!obj->funcs->vm_ops) { + ret = -EINVAL; + goto err_drm_gem_object_put; + } + vma->vm_ops = obj->funcs->vm_ops; + if (obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); - if (ret) { - drm_gem_object_put(obj); - return ret; - } + if (ret) + goto err_drm_gem_object_put; WARN_ON(!(vma->vm_flags & VM_DONTEXPAND)); } else { - if (obj->funcs->vm_ops) - vma->vm_ops = obj->funcs->vm_ops; - else { - drm_gem_object_put(obj); - return -EINVAL; - } - vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); } return 0; + +err_drm_gem_object_put: + drm_gem_object_put(obj); + return ret; } EXPORT_SYMBOL(drm_gem_mmap_obj); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 683aa29ecd3b..0549ec190583 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -717,6 +717,10 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); if (obj->funcs && obj->funcs->mmap) { + if (!obj->funcs->vm_ops) + return -EINVAL; + vma->vm_ops = obj->funcs->vm_ops; + ret = obj->funcs->mmap(obj, vma); if (ret) return ret;
The GEM mmap code relies on the GEM object's mmap callback to set the VMA's vm_ops field. This is easily forgotten and lead to a memory leak in the CMA helpers. Instead set the vm_ops field in the DRM core code to the GEM object's value. Drivers with different needs can override this in their mmap callback. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Fixes: f5ca8eb6f9bd ("drm/cma-helper: Implement mmap as GEM CMA object functions") Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Eric Anholt <eric@anholt.net> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_gem.c | 23 ++++++++++++----------- drivers/gpu/drm/drm_prime.c | 4 ++++ 2 files changed, 16 insertions(+), 11 deletions(-)