lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v6 5/8] drm/i915: Check for integer truncation on the configuration of ttm place
From


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

\
 
 \ /
  Last update: 2022-08-16 13:20    [W:0.063 / U:1.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site