Re: [PATCH v5 4/7] drm/i915: Check for integer truncation on the configuration of ttm place

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

On Mon, 25 Jul 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()
>
> 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>
> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c |  6 +++---
>  drivers/gpu/drm/i915/i915_gem.h         |  4 ++++
>  drivers/gpu/drm/i915/intel_region_ttm.c | 20 +++++++++++++++++---
>  3 files changed, 24 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..88f2887627dc 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);
> +		safe_conversion_gem_bug_on(&place->fpfn, offset >> PAGE_SHIFT);
> +		safe_conversion_gem_bug_on(&place->lpfn, place->fpfn + (size >> PAGE_SHIFT));

So the natural thing would be to have and use two orthogonal helpers, a
safe_conversion predicate and a warn:

	GEM_BUG_ON(!safe_conversion(...));

or even:

	if (GEM_BUG_ON(!safe_conversion(...)))
		/* ... */

But GEM_BUG_ON() is surprising and does not follow the same pattern as
WARN_ON/BUG_ON. *sigh*

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;
> +			safe_conversion_gem_bug_on(&place->lpfn, mr->io_size >> PAGE_SHIFT);
>  		}
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index 68d8d52bd541..327dacedd5d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -83,5 +83,9 @@ struct drm_i915_private;
>  #endif
>  
>  #define I915_GEM_IDLE_TIMEOUT (HZ / 5)
> +#define safe_conversion_gem_bug_on(ptr, value) !({ \
> +	safe_conversion(ptr, value) ? 0 \
> +		: (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 1); \
> +})
>  
>  #endif /* __I915_GEM_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
> index 575d67bc6ffe..f0d143948725 100644
> --- a/drivers/gpu/drm/i915/intel_region_ttm.c
> +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
> @@ -209,14 +209,26 @@ 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_gem_bug_on(&place.fpfn,
> +						offset >> PAGE_SHIFT)) {
> +			ret = -E2BIG;
> +			goto out;
> +		}
> +		if (!safe_conversion_gem_bug_on(&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_gem_bug_on(&place.lpfn,
> +							mem->io_size >> PAGE_SHIFT)) {
> +				ret = -E2BIG;
> +				goto out;
> +			}
>  		}
>  	}
>  
> @@ -224,6 +236,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)

-- 
Jani Nikula, Intel Open Source Graphics Center