From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 10/16] drm/gem: implement mmap access management Date: Tue, 13 Aug 2013 23:05:53 +0200 Message-ID: <20130813210553.GI9296@phenom.ffwll.local> References: <1376422717-12229-1-git-send-email-dh.herrmann@gmail.com> <1376422717-12229-11-git-send-email-dh.herrmann@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f173.google.com (mail-ea0-f173.google.com [209.85.215.173]) by gabe.freedesktop.org (Postfix) with ESMTP id 9762DE5F8C for ; Tue, 13 Aug 2013 14:05:45 -0700 (PDT) Received: by mail-ea0-f173.google.com with SMTP id g10so4403427eak.18 for ; Tue, 13 Aug 2013 14:05:44 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1376422717-12229-11-git-send-email-dh.herrmann@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: David Herrmann Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Tue, Aug 13, 2013 at 09:38:31PM +0200, David Herrmann wrote: > Implement automatic access management for mmap offsets for all GEM > drivers. This prevents user-space applications from "guessing" GEM BO > offsets and accessing buffers which they don't own. > > TTM drivers or other modesetting drivers with custom mm handling might > make use of GEM but don't need its mmap helpers. To avoid unnecessary > overhead, we limit GEM access management to drivers using DRIVER_GEM_MMAP. > So for TTM drivers GEM will not call any *_allow() or *_revoke() helpers. > > Signed-off-by: David Herrmann Overall I'm not sold why we need the GEM_MMAP feature flag. Drivers that don't use drm_gem_mmap shouldn't get hurt with it if we ignore the little bit of useless overhead due to obj->vma_node. But they already have that anyway with obj->vma, so meh. And more incentives to move ttm over to the gem object manager completely ;-) One comment below. Cheers, Daniel > --- > Documentation/DocBook/drm.tmpl | 13 +++++++++++++ > drivers/gpu/drm/drm_gem.c | 36 ++++++++++++++++++++++++++++++++---- > include/drm/drmP.h | 1 + > 3 files changed, 46 insertions(+), 4 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index 87e22ec..a388749 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -223,6 +223,19 @@ > > > > + DRIVER_GEM_MMAP > + > + Driver uses default GEM mmap helpers. This flag guarantees that > + GEM core takes care of buffer access management and prevents > + unprivileged users from mapping random buffers. This flag should > + only be set by GEM-only drivers that use the drm_gem_mmap_*() > + helpers directly. TTM, on the other hand, takes care of access > + management itself, even though drivers might use DRIVER_GEM and > + TTM at the same time. See the DRM VMA Offset Manager interface for > + more information on buffer mmap() access management. > + > + > + > DRIVER_MODESET > > Driver supports mode setting interfaces (KMS). > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 7043d89..887274f 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -236,6 +236,9 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle) > > drm_gem_remove_prime_handles(obj, filp); > > + if (drm_core_check_feature(dev, DRIVER_GEM_MMAP)) > + drm_vma_node_revoke(&obj->vma_node, filp->filp); > + > if (dev->driver->gem_close_object) > dev->driver->gem_close_object(obj, filp); > drm_gem_object_handle_unreference_unlocked(obj); > @@ -288,15 +291,26 @@ drm_gem_handle_create(struct drm_file *file_priv, > > drm_gem_object_handle_reference(obj); > > + if (drm_core_check_feature(dev, DRIVER_GEM_MMAP)) { > + ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp); > + if (ret) > + goto err_handle; > + } > + > if (dev->driver->gem_open_object) { > ret = dev->driver->gem_open_object(obj, file_priv); > - if (ret) { > - drm_gem_handle_delete(file_priv, *handlep); > - return ret; > - } > + if (ret) > + goto err_node; The error handling cleanup changes here aren't required since handle_delete will dtrt anyway. Or at least I hope it does ;-) -Daniel > } > > return 0; > + > +err_node: > + if (drm_core_check_feature(dev, DRIVER_GEM_MMAP)) > + drm_vma_node_revoke(&obj->vma_node, file_priv->filp); > +err_handle: > + drm_gem_handle_delete(file_priv, *handlep); > + return ret; > } > EXPORT_SYMBOL(drm_gem_handle_create); > > @@ -486,6 +500,9 @@ drm_gem_object_release_handle(int id, void *ptr, void *data) > > drm_gem_remove_prime_handles(obj, file_priv); > > + if (drm_core_check_feature(dev, DRIVER_GEM_MMAP)) > + drm_vma_node_revoke(&obj->vma_node, file_priv->filp); > + > if (dev->driver->gem_close_object) > dev->driver->gem_close_object(obj, file_priv); > > @@ -610,6 +627,10 @@ EXPORT_SYMBOL(drm_gem_vm_close); > * the GEM object is not looked up based on its fake offset. To implement the > * DRM mmap operation, drivers should use the drm_gem_mmap() function. > * > + * drm_gem_mmap_obj() assumes the user is granted access to the buffer while > + * drm_gem_mmap() prevents unprivileged users from mapping random objects. So > + * callers must verify access restrictions before calling this helper. > + * > * NOTE: This function has to be protected with dev->struct_mutex > * > * Return 0 or success or -EINVAL if the object size is smaller than the VMA > @@ -658,6 +679,9 @@ EXPORT_SYMBOL(drm_gem_mmap_obj); > * Look up the GEM object based on the offset passed in (vma->vm_pgoff will > * contain the fake offset we created when the GTT map ioctl was called on > * the object) and map it with a call to drm_gem_mmap_obj(). > + * > + * If the caller is not granted access to the buffer object, the mmap will fail > + * with EACCES. Please see DRIVER_GEM_MMAP for more information. > */ > int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) > { > @@ -678,6 +702,10 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) > if (!node) { > mutex_unlock(&dev->struct_mutex); > return drm_mmap(filp, vma); > + } else if (drm_core_check_feature(dev, DRIVER_GEM_MMAP) && > + !drm_vma_node_is_allowed(node, filp)) { > + mutex_unlock(&dev->struct_mutex); > + return -EACCES; > } > > obj = container_of(node, struct drm_gem_object, vma_node); > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 3ecdde6..d51accd 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -152,6 +152,7 @@ int drm_err(const char *func, const char *format, ...); > #define DRIVER_GEM 0x1000 > #define DRIVER_MODESET 0x2000 > #define DRIVER_PRIME 0x4000 > +#define DRIVER_GEM_MMAP 0x8000 > > #define DRIVER_BUS_PCI 0x1 > #define DRIVER_BUS_PLATFORM 0x2 > -- > 1.8.3.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch