Re: [Intel-gfx] [PATCH] drm/i915: Fix possible null ptr dereferences
On Fri, 03 Dec 2021, Thomas Hellström 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,
>
> 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(>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(>_dst_iter.tt, dst_ttm)
>> > :
>> > --
>> > 2.25.1
>>
>
>
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915: Fix possible null ptr dereferences
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,
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.
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(>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(>_dst_iter.tt, dst_ttm)
> > :
> > --
> > 2.25.1
>
Re: [Intel-gfx] [PATCH] drm/i915: Fix possible null ptr dereferences
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
> ---
> 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(>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(>_dst_iter.tt, dst_ttm) :
> --
> 2.25.1
--
Ville Syrjälä
Intel
