Re: [Intel-gfx] [PATCH] drm/i915/ttm: Fix access_memory null pointer exception

[Andi Shyti <andi.shyti@linux.intel.com>]

On Fri, Oct 14, 2022 at 09:39:52AM +0100, Matthew Auld wrote:
> On 13/10/2022 18:56, Jonathan Cavitt wrote:
> > i915_ttm_to_gem can return a NULL pointer, which is
> > dereferenced in i915_ttm_access_memory without first
> > checking if it is NULL.  Inspecting
> > i915_ttm_io_mem_reserve, it appears the correct
> > behavior in this case is to return -EINVAL.
> 
> The GEM object has already been dereferenced before this point, if you look
> at the caller (vm_access_ttm). The NULL obj thing is to identify "ttm ghost
> objects", and I don't think a normal userpace object can suddenly become one
> (access_memory comes from ptrace). AFAIK ghost objects are just for
> temporarily hanging on to some memory/state, while the dma-resv is busy. In
> the places where ttm is the one giving us the object, then it might be
> possible to see these types of objects, since ttm could in theory pass one
> in (like during eviction).

True that, but because from a code persepctive we can still receive
NULL, I think the check is correct, perhaps we could:

	if (unlikely(!obj))
		return -EINVAL;

Andi

> > Fixes: 26b15eb0 ("drm/i915/ttm: implement access_memory")
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > Suggested-by: John C Harrison <John.C.Harrison@intel.com>
> > CC: Matthew Auld <matthew.auld@intel.com>
> > CC: Andrzej Hajda <andrzej.hajda@intel.com>
> > CC: Nirmoy Das <nirmoy.das@intel.com>
> > CC: Andi Shyti <andi.shyti@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > index d63f30efd631..b569624f2ed9 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > @@ -704,11 +704,16 @@ static int i915_ttm_access_memory(struct ttm_buffer_object *bo,
> >   				  int len, int write)
> >   {
> >   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> > -	resource_size_t iomap = obj->mm.region->iomap.base -
> > -		obj->mm.region->region.start;
> > +	resource_size_t iomap;
> >   	unsigned long page = offset >> PAGE_SHIFT;
> >   	unsigned long bytes_left = len;
> > +	if (!obj)
> > +		return -EINVAL;
> > +
> > +	iomap = obj->mm.region->iomap.base -
> > +		obj->mm.region->region.start;
> > +
> >   	/*
> >   	 * TODO: For now just let it fail if the resource is non-mappable,
> >   	 * otherwise we need to perform the memcpy from the gpu here, without