Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <92e5ebbd-9af1-c7e3-6d9e-7ea2958ad9c8@intel.com>
Date:   Tue, 16 Aug 2022 18:42:12 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v6 5/8] drm/i915: Check for integer truncation on the
 configuration of ttm place
Content-Language: en-US
To:     Jani Nikula <jani.nikula@intel.com>,
        <intel-gfx@lists.freedesktop.org>
CC:     <linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <mchehab@kernel.org>, <chris@chris-wilson.co.uk>,
        <matthew.auld@intel.com>, <thomas.hellstrom@linux.intel.com>,
        <nirmoy.das@intel.com>, <airlied@linux.ie>, <daniel@ffwll.ch>,
        <andi.shyti@linux.intel.com>, <andrzej.hajda@intel.com>
References: <20220813010857.4043956-1-gwan-gyeong.mun@intel.com>
 <20220813010857.4043956-6-gwan-gyeong.mun@intel.com>
 <877d3arl0u.fsf@intel.com>
From:   Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
In-Reply-To: <877d3arl0u.fsf@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ek1JMkQ2K25XR1F5MTZVNlp3VFpFY0FVQkIwUlJob2tHSUJxWDNzWGtJRldR?=
 =?utf-8?B?NG15enBqWm82VEF2T1kwZzFmS25XcmFpS01rRWNXQjNuT2JnUEtXekVhKzFp?=
 =?utf-8?B?WEI4Yk1DbEwvajdFdE82THlyN1JEcVQwZFpFSnRUWkp6ZGFZb0RUYk9pNURx?=
 =?utf-8?B?NjJINzFBUDgwU1g2R1d5bGxES3UwRUVPcGtUYWVmdlZwRzBzdjhhMEh6a3hJ?=
 =?utf-8?B?VFhrY1JoTGd1YTRNNXVHR1d6aGZOV09rRCtNb1Z6ZVhZYWNsNmlUUTNzK1cz?=
 =?utf-8?B?RFU4MGEycTN0ZTBYRTJTRnBnZEkyakY5dUtZLzloT09kQnYxK0tMRnZacFpL?=
 =?utf-8?B?ampub1EyWmZpMzRGeW95emsrMnhBQTdUeGpEVkxyMTA2SVl0UU5MeU9aQ1Jt?=
 =?utf-8?B?REIyeDV1dUl4OWhTZVlsN0lQL2NQQjQ0MkppT2ZiV1VqY0s2Z3F4QVpURHV1?=
 =?utf-8?B?dG9COGl6eEpIb1AyV1p1Q0FFM3ZuYWhCbUpVQkVqelYyY1RIVEcyY0x4ekVG?=
 =?utf-8?B?eldoK0k4VGlsSXlMQnRVb1c3VGlyQ0JOOW5IVkZleVNYaTRlRHNYNGNMakZq?=
 =?utf-8?B?czM4b0pDUlRyZWxBelJxNUMxMTZHb1RRdG5hZTlMcnB1dGdBS3BRcy9lZ3d2?=
 =?utf-8?B?NDY4b0lLbjQ1WEhLM2YvK0V3VmlhRjJlSkp0Wnl2VjVsVU82M3JJdmpHNVZz?=
 =?utf-8?B?VitwUUxaSktxN3BDbjFjb3FIMEJqNy9iaVZrOVZ2Yk5hcUpINkpyMXFtT3o0?=
 =?utf-8?B?RitHdkxUSTVibVJEWXVpdXhwNTZtZ0wxNkFmZ0Y2a3BrSnlkU0lxOER2cVBo?=
 =?utf-8?B?bE5CQU9PdnNYWlJzNTBUUWRENEZhaG9aUTFWRXdhakpHYWVxUmVIRDJ2YXFm?=
 =?utf-8?B?d01LMnovZ2p3N04vTFRKKzJORndOdWovb1RVY2dEOGFmWVM1OGs3M1dZc213?=
 =?utf-8?B?WURNRHRWd2R5bzNrM1lVWURadk9FbGpVTnhjUm1IVStVSlI3VEdCWjBXbXZ1?=
 =?utf-8?B?R1R5b2gyRm4xYUhLQUhkb09raGtOYXNHbzV2aGN5NFo3NjdzemVOWGJrdDdU?=
 =?utf-8?B?dSsxa0RNQWxtdW1EOElMQzMvdDZZVmVYd3ZnNTJsTGdKTlFFU2xVQUhuMUpk?=
 =?utf-8?B?ZFlEaFhiVnJlOGR4aGFINW0wTG42TEFqYzFwUnhiU3hseXdSNm5mT2d3TXI0?=
 =?utf-8?B?dzNCWkU4cFZtaE9rVllUZW00VUlhNnZqTUJkeTFoMW5KbFllNVVDVktlZjdm?=
 =?utf-8?B?d2czUm56RXo5eHZMQ0RqTHhXYWI5SFVoTXo5NjZsMVE4NlJXWnVIMGh4djVp?=
 =?utf-8?B?Y3ZUSk9MTFlFM2N3VWZMSEVlemJHcDArZ2NLYjlRdzVYOFJmRFpQR2pVeGwy?=
 =?utf-8?B?Zk9vOHA1Mk1tT2NvTlFSVXF1VW9YcEtQLzc0SytnUm5SSkM5b2VvZUQxMzBY?=
 =?utf-8?B?WE12TDZJVnM0QTV0MTRsRnFkeFFjVmhlSnlBbzk3N1h3MlI1eW11ais0SmtE?=
 =?utf-8?B?enFTeENZb3VRbC9RbElLQTE0ZFg3S1ltZmpYUEwzcjFTbkxJRlVpU2REZWYx?=
 =?utf-8?B?Z2YvTjhYdzl0dTJvWTJiQW9va2p2TWlaTlQzWXF2MFFRZ2w1cVJuYUllN291?=
 =?utf-8?B?cnlkbkdtTlgvNm0zOG9oMndKTGNrNWJ1MlJSL2pIenlDUURMUTVqUUxhZHBC?=
 =?utf-8?B?QTJvK2lhQXpIU05xWXpRNDdaM2NUKzZnRk5PeFQ2NTNiczE4Q1c3aFRMcElw?=
 =?utf-8?B?SU9JL0tWWkV5UHRzUDNGYkhCQjg2QWhxaGxEdUtva0dSdUxLbmdaOUxwSW90?=
 =?utf-8?B?K1VuT1RHNHRlQ2dIajlXTWVhOFFUOFBRRVNrQ2dEa0g2UEZ3TmMzM2g5eGFz?=
 =?utf-8?B?UjN2REpTQmhLaC9OaUZwUmJEU1Z3MmJYdHAyRFhyd0VlbHZUR2lyVWNtUEpI?=
 =?utf-8?B?MGY1akdnbWVObHVTUElWMkpmd0pITGc4cmFZWFcvUVhlS3VOaVlBa25tMHNV?=
 =?utf-8?B?KzRVRENjaVVHMUE2RExMRTVlL1psREtwSWFrbVJ2V1R5MHpueUtxV3ZBcXA2?=
 =?utf-8?B?TzRKWlhiTHFJRW5BV2d5MXJha0dweThhbzc3K0NLbGlnZWtlamh6c1d0SHBW?=
 =?utf-8?B?RERmN0lwME55TlhLa0NLMWtvZEVuTHFWY0VyTk41MnhzZnVISUJDMmJaSWxt?=
 =?utf-8?B?SGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 69e7c232-b4cd-4647-b628-08da7f6ba2fc
X-MS-Exchange-CrossTenant-AuthSource: CH0PR11MB5409.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Aug 2022 09:42:30.2169
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1f1gjzYf9NBhPxV973N+n9p+sDM0s4s7/W+MPWMZATH+5PoD8kVrZ/Uo+y/096fiy9PGR6HAyRtSkST6A6g/by8jBGx8M3CpGp1bKr5cCHE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB0044
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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