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

["Das, Nirmoy" <nirmoy.das@intel.com>]

Hi Matt

On 10/14/2022 10:39 AM, 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).


Yes, we should not hit this.  Thanks for the nice "ttm ghost objects" 
reminder :)


I think we can still have this check to avoid code analysis tool 
warnings, what do you think ?


Thanks,

Nirmoy

>
>>
>> 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