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. 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) -- Jani Nikula, Intel Open Source Graphics Center