Re: [Intel-gfx] [PATCH] drm/i915: Fix possible null ptr dereferences

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 03 Dec 2021, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote:
> On Fri, 2021-12-03 at 13:02 +0200, Ville Syrjälä wrote:
>> On Fri, Dec 03, 2021 at 12:02:57PM +0530, Pallavi Mishra wrote:
>> > add null ptr checks to prevent crash/exceptions.
>> 
>> BUG_ON()s aren't going to fix anything.
>> 
>> > Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com>
>
> Pallavi, 
>
> The NULL pointer dereferences here are probably all false positives
> from a static analyzer. However the GEM_BUG_ONs are fine to assert that
> the assumption really holds and to clearly point out what's going wrong
> if they are hit in CI tests.

I think we're massively overusing GEM_BUG_ON() all over the place.

BR,
Jani.



>
> But the commit message must reflect that.
>
> /Thomas.
>
>
>> > ---
>> >  drivers/gpu/drm/i915/gem/i915_gem_ttm.c      | 3 +++
>> >  drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 ++-
>> >  2 files changed, 5 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > index 218a9b3037c7..997fe73c205b 100644
>> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > @@ -906,6 +906,8 @@ vm_access_ttm(struct vm_area_struct *area,
>> > unsigned long addr,
>> >         struct drm_i915_gem_object *obj =
>> >                 i915_ttm_to_gem(area->vm_private_data);
>> >  
>> > +       GEM_BUG_ON(!obj);
>> > +
>> >         if (i915_gem_object_is_readonly(obj) && write)
>> >                 return -EACCES;
>> >  
>> > @@ -966,6 +968,7 @@ static const struct drm_i915_gem_object_ops
>> > i915_gem_ttm_obj_ops = {
>> >  void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
>> >  {
>> >         struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>> > +       GEM_BUG_ON(!obj);
>> >  
>> >         i915_gem_object_release_memory_region(obj);
>> >         mutex_destroy(&obj->ttm.get_io_page.lock);
>> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > index 80df9f592407..2b684903a9f5 100644
>> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > @@ -371,6 +371,7 @@ int i915_ttm_move_notify(struct
>> > ttm_buffer_object *bo)
>> >         struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>> >         int ret;
>> >  
>> > +       GEM_BUG_ON(!obj);
>> >         ret = i915_gem_object_unbind(obj,
>> > I915_GEM_OBJECT_UNBIND_ACTIVE);
>> >         if (ret)
>> >                 return ret;
>> > @@ -506,7 +507,7 @@ static void i915_ttm_memcpy_init(struct
>> > i915_ttm_memcpy_arg *arg,
>> >  
>> >         dst_reg = i915_ttm_region(bo->bdev, dst_mem->mem_type);
>> >         src_reg = i915_ttm_region(bo->bdev, bo->resource-
>> > >mem_type);
>> > -       GEM_BUG_ON(!dst_reg || !src_reg);
>> > +       GEM_BUG_ON(!dst_reg || !src_reg || !obj);
>> >  
>> >         arg->dst_iter = !i915_ttm_cpu_maps_iomem(dst_mem) ?
>> >                 ttm_kmap_iter_tt_init(&arg->_dst_iter.tt, dst_ttm)
>> > :
>> > -- 
>> > 2.25.1
>> 
>
>

-- 
Jani Nikula, Intel Open Source Graphics Center