linux-kernel - Re: [PATCH v2 4/5] drm/i915: ttm backend dont provide mmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Apr 2022 16:05:00 +0200
From:   Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>
To:     Robert Beckett <bob.beckett@...labora.com>,
        dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     Matthew Auld <matthew.auld@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/5] drm/i915: ttm backend dont provide mmap_offset
 for kernel buffers

On Tue, 2022-04-12 at 15:18 +0000, Robert Beckett wrote:
> stolen/kernel buffers should not be mmapable by userland.
> do not provide callbacks to facilitate this for these buffers.
> 
> Signed-off-by: Robert Beckett <bob.beckett@...labora.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 32 +++++++++++++++++++++--
> --
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index a878910a563c..b20f81836c54 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -1092,8 +1092,8 @@ static void i915_ttm_unmap_virtual(struct
> drm_i915_gem_object *obj)
>         ttm_bo_unmap_virtual(i915_gem_to_ttm(obj));
>  }
>  
> -static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
> -       .name = "i915_gem_object_ttm",
> +static const struct drm_i915_gem_object_ops
> i915_gem_ttm_user_obj_ops = {
> +       .name = "i915_gem_object_ttm_user",
>         .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
>                  I915_GEM_OBJECT_SELF_MANAGED_SHRINK_LIST,
>  
> @@ -1111,6 +1111,21 @@ static const struct drm_i915_gem_object_ops
> i915_gem_ttm_obj_ops = {
>         .mmap_ops = &vm_ops_ttm,
>  };
>  
> +static const struct drm_i915_gem_object_ops
> i915_gem_ttm_kern_obj_ops = {
> +       .name = "i915_gem_object_ttm_kern",
> +       .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
> +                I915_GEM_OBJECT_SELF_MANAGED_SHRINK_LIST,
> +
> +       .get_pages = i915_ttm_get_pages,
> +       .put_pages = i915_ttm_put_pages,
> +       .truncate = i915_ttm_truncate,
> +       .shrink = i915_ttm_shrink,
> +
> +       .adjust_lru = i915_ttm_adjust_lru,
> +       .delayed_free = i915_ttm_delayed_free,
> +       .migrate = i915_ttm_migrate,
> +};

Do we really need two different ops here?

Since if we don't have mmap ops, basically that tells GEM it should do
the mmapping rather than TTM. 

That might of course come in handy for the shmem backend, but I don't
fully follow why we need this for stolen.

Also for the framebuffer handed over from BIOS to fbdev, Does that need
mmapping and if so, how do we handle that?


/Thomas




> +
>  void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
>  {
>         struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> @@ -1165,10 +1180,19 @@ int __i915_gem_ttm_object_init(struct
> intel_memory_region *mem,
>                 .no_wait_gpu = false,
>         };
>         enum ttm_bo_type bo_type;
> +       const struct drm_i915_gem_object_ops *ops;
>         int ret;
>  
>         drm_gem_private_object_init(&i915->drm, &obj->base, size);
> -       i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class,
> flags);
> +
> +       if (flags & I915_BO_ALLOC_USER &&
> intel_region_to_ttm_type(mem) != I915_PL_STOLEN) {
> +               bo_type = ttm_bo_type_device;
> +               ops = &i915_gem_ttm_user_obj_ops;
> +       } else {
> +               bo_type = ttm_bo_type_kernel;
> +               ops = &i915_gem_ttm_kern_obj_ops;
> +       }
> +       i915_gem_object_init(obj, ops, &lock_class, flags);
>  
>         obj->bo_offset = offset;
>  
> @@ -1178,8 +1202,6 @@ int __i915_gem_ttm_object_init(struct
> intel_memory_region *mem,
>  
>         INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL |
> __GFP_NOWARN);
>         mutex_init(&obj->ttm.get_io_page.lock);
> -       bo_type = (obj->flags & I915_BO_ALLOC_USER) ?
> ttm_bo_type_device :
> -               ttm_bo_type_kernel;
>  
>         obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
>