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

[Gwan-gyeong Mun]

On 8/15/22 5:03 PM, Jani Nikula wrote:
> On Sat, 13 Aug 2022, Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> > Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Reviewed-by: Nirmoy Das <nirmoy.das@xxxxxxxxx> (v2)
> > Reviewed-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> (v3)
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx> (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)