Re: [PATCH v6 5/8] drm/i915: Check for integer truncation on the configuration of ttm place

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

On 8/15/22 5:03 PM, Jani Nikula wrote:
> On Sat, 13 Aug 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
>> There is an impedance mismatch between the first/last valid page
>> frame number of ttm place in unsigned and our memory/page accounting in
>> unsigned long.
>> As the object size is under the control of userspace, we have to be prudent
>> and catch the conversion errors.
>> To catch the implicit truncation as we switch from unsigned long to
>> unsigned, we use overflows_type check and report E2BIG or overflow_type
>> prior to the operation.
>>
>> v3: Not to change execution inside a macro. (Mauro)
>>      Add safe_conversion_gem_bug_on() macro and remove temporal
>>      SAFE_CONVERSION() macro.
>> v4: Fix unhandled GEM_BUG_ON() macro call from safe_conversion_gem_bug_on()
>> v6: Fix to follow general use case for GEM_BUG_ON(). (Jani)
>>
>> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com> (v2)
>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v3)
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> (v5)
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c |  6 +++---
>>   drivers/gpu/drm/i915/intel_region_ttm.c | 22 +++++++++++++++++++---
>>   2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 9f2be1892b6c..30f488712abe 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -140,14 +140,14 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr,
>>   	if (flags & I915_BO_ALLOC_CONTIGUOUS)
>>   		place->flags |= TTM_PL_FLAG_CONTIGUOUS;
>>   	if (offset != I915_BO_INVALID_OFFSET) {
>> -		place->fpfn = offset >> PAGE_SHIFT;
>> -		place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
>> +		GEM_BUG_ON(!safe_conversion(&place->fpfn, offset >> PAGE_SHIFT));
>> +		GEM_BUG_ON(!safe_conversion(&place->lpfn, place->fpfn + (size >> PAGE_SHIFT)));
> 
> This would be the natural thing to do with BUG_ON/WARN_ON. And I'd like
> it if we could use it like this. But, as I tried to say, GEM_BUG_ON is
> nothing like BUG_ON/WARN_ON, and no code is generated for
> CONFIG_DRM_I915_DEBUG_GEM=n. And our CI will never catch it because it
> always has CONFIG_DRM_I915_DEBUG_GEM=y.
> 
Hi Jani,

Thanks for the detailed explanation of what the build option 
CONFIG_DRM_I915_DEBUG_GEM doesn't cover.

Using the WARN_ON() macro, I modified with the way in your comments on 
v5 version and sent the v7 patch again.

Many thanks,

G.G

> BR,
> Jani.
> 
> 
>>   	} else if (mr->io_size && mr->io_size < mr->total) {
>>   		if (flags & I915_BO_ALLOC_GPU_ONLY) {
>>   			place->flags |= TTM_PL_FLAG_TOPDOWN;
>>   		} else {
>>   			place->fpfn = 0;
>> -			place->lpfn = mr->io_size >> PAGE_SHIFT;
>> +			GEM_BUG_ON(!safe_conversion(&place->lpfn, mr->io_size >> PAGE_SHIFT));
>>   		}
>>   	}
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
>> index 575d67bc6ffe..c480b0b50bcc 100644
>> --- a/drivers/gpu/drm/i915/intel_region_ttm.c
>> +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
>> @@ -209,14 +209,28 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
>>   	if (flags & I915_BO_ALLOC_CONTIGUOUS)
>>   		place.flags |= TTM_PL_FLAG_CONTIGUOUS;
>>   	if (offset != I915_BO_INVALID_OFFSET) {
>> -		place.fpfn = offset >> PAGE_SHIFT;
>> -		place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
>> +		if (!safe_conversion(&place.fpfn, offset >> PAGE_SHIFT)) {
>> +			GEM_BUG_ON(!safe_conversion(&place.fpfn,offset >> PAGE_SHIFT));
>> +			ret = -E2BIG;
>> +			goto out;
>> +		}
>> +		if (!safe_conversion(&place.lpfn, place.fpfn + (size >> PAGE_SHIFT))) {
>> +			GEM_BUG_ON(!safe_conversion(&place.lpfn,
>> +						    place.fpfn + (size >> PAGE_SHIFT)));
>> +			ret = -E2BIG;
>> +			goto out;
>> +		}
>>   	} else if (mem->io_size && mem->io_size < mem->total) {
>>   		if (flags & I915_BO_ALLOC_GPU_ONLY) {
>>   			place.flags |= TTM_PL_FLAG_TOPDOWN;
>>   		} else {
>>   			place.fpfn = 0;
>> -			place.lpfn = mem->io_size >> PAGE_SHIFT;
>> +			if (!safe_conversion(&place.lpfn, mem->io_size >> PAGE_SHIFT)) {
>> +				GEM_BUG_ON(!safe_conversion(&place.lpfn,
>> +							    mem->io_size >> PAGE_SHIFT));
>> +				ret = -E2BIG;
>> +				goto out;
>> +			}
>>   		}
>>   	}
>>   
>> @@ -224,6 +238,8 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
>>   	mock_bo.bdev = &mem->i915->bdev;
>>   
>>   	ret = man->func->alloc(man, &mock_bo, &place, &res);
>> +
>> +out:
>>   	if (ret == -ENOSPC)
>>   		ret = -ENXIO;
>>   	if (!ret)
>