Re: [PATCH 2/3] drm/xe/vf: Remove lmtt->ops null check in xe_lmtt_estimate_pt_size

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 08-03-2024 20:22, Rodrigo Vivi wrote:
> On Fri, Mar 08, 2024 at 10:06:50AM +0530, Himal Prasad Ghimiray wrote:
>> In xe_lmtt_estimate_pt_size: Pointer is checked against null but then
>> dereferenced anyway.
> And what's the problem?
>
> In the line below it access beyond this pointer, so it is a fair
> case.

The problem is even if it is NULL it will be  try to derefrence it. 
Which might lead to segmentation fault.

>
>> Since xe_lmtt_init ensures lmtt->ops is populated
>> remove the check.
> With this in mind we could simply remove all the asserts in the code.
>
> I believe that if someone introduced it here it is likely because
> during some development or refactor this ended up being a problem
> and want some earlier kind of warning with backtrace information.
>
>> Reported by static analyzer.
> Perhaps then replace with an
> if (!lmtt->ops) {
>      drm_WARN(...);
>      return;
> }

I am also of the opinion that this is the correct check to have instead 
of just warning

about lmtt->ops being NULL and continue to dereference it. Need clarity 
on what should we return in

case of lmtt->ops being NULL since expected return type is u64.

>
> and/or mark the tool as a false positive?!
>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_lmtt.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_lmtt.c b/drivers/gpu/drm/xe/xe_lmtt.c
>> index 0d7c5514e092..d6d75414bb99 100644
>> --- a/drivers/gpu/drm/xe/xe_lmtt.c
>> +++ b/drivers/gpu/drm/xe/xe_lmtt.c
>> @@ -487,7 +487,6 @@ u64 xe_lmtt_estimate_pt_size(struct xe_lmtt *lmtt, u64 size)
>>   
>>   	lmtt_assert(lmtt, IS_SRIOV_PF(lmtt_to_xe(lmtt)));
>>   	lmtt_assert(lmtt, IS_DGFX(lmtt_to_xe(lmtt)));
>> -	lmtt_assert(lmtt, lmtt->ops);
>>   
>>   	pt_size = PAGE_ALIGN(lmtt->ops->lmtt_pte_size(level) *
>>   			     lmtt->ops->lmtt_pte_num(level));
>> -- 
>> 2.25.1
>>