Re: [PATCH] drm/xe/xe_migrate: Fix potential overflows expression

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

[-- Attachment #1: Type: text/plain, Size: 7401 bytes --]


On 23-03-2024 01:07, Rodrigo Vivi wrote:
> On Fri, Mar 22, 2024 at 03:00:17PM +0530, Ghimiray, Himal Prasad wrote:
>>     On 21-03-2024 02:27, Rodrigo Vivi wrote:
>>                                                                                  
>>     On Wed, Mar 20, 2024 at 03:48:35PM +0530, Himal Prasad Ghimiray wrote:
>>                                                                                  
>>     >Cast to proper datatypes to avoid overflows.
>>                                                                                  
>>     I'm afraid that the cast wont prevent the overflow, but mask it.
>>     probably safer to move the multiplication to some of the helpers
>>     in linux/math64.h ?!
>>                                                                                  
>>     Hi Rodrigo,
>>     Thank you for your response. The modifications are inspired by the inline
>>     u64 mul_u32_u32(u32 a, u32 b)
>>     function defined in linux/math64.h. Initially, I considered using the same
>>     approach.
>>     However, I discovered an architecture-specific implementation for
>>     mul_u32_u32.
>>     To prevent ambiguity, I opted for casting, which I observed is a standard
>>     practice throughout Linux code.
> afaik the cast just tells compilers and static analyzer tools that we know
> what we are doing and we know that that math on the right won't exceed
> the cast size. But it doesn't prevent overflow. You need to take care
> and be sure that you are not overflowing the bits you have available. Always.

Apologies for the oversight. I realize now that I should have been 
clearer in the commit message regarding the specific overflows I aimed 
to address.

In this context, we are performing multiplication of two 32-bit operands 
and expecting the result as a |u64|. However, not all compilers or 
platforms are guaranteed to use a |u64| for the intermediate result of 
multiplication in this scenario. Some might opt for a |u32| for storing 
the intermediate result of multiplication before widening it to |u64|.

By explicitly casting one of the operands to |u64|, we ensure that the 
multiplication will be carried out with a |u64| as the intermediate 
result. This casting mitigates the risk of overflow that might occur due 
to the multiplication of two lower precision (|u32|) operands before 
widening the result to higher precision (|u64|).

This patch does not aim to address overflow if the result itself 
overflows |u64|, but rather focuses on addressing overflow that might 
occur due to the multiplication of two lower precision (|u32|) operands 
before widening the result to higher precision (|u64|).

>
> I doubled checked all the cases below and I'm sure we don't have any
> overflow issue there. So you need to at least adjust the commit message.

Will modify the commit message to:

"Addressing potential overflow in result of  multiplication of two lower 
precision (|u32|) operands

  before widening it to higher precision (|u64|)."

>
>
> Btw, (map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) probably deserves a separate
> patch to make it to use some of the variants of DIV_ROUND macros...

To me this looks unnecessary.  But if you feel it is good to have or 
required it can be done.

BR
Himal Ghimiray

>
>
>>     BR
>>     Himal
>>                                                                                  
>>                                                                                  
>>                                                                                  
>>     >Cc: Matthew Auld [1]<matthew.auld@intel.com>                                 
>>     >Cc: Matthew Brost [2]<matthew.brost@intel.com>                               
>>     >Cc: Rodrigo Vivi [3]<rodrigo.vivi@intel.com>                                 
>>     >Signed-off-by: Himal Prasad Ghimiray [4]<himal.prasad.ghimiray@intel.com>    
>>     >---
>>     >These errors were highlighted by Coverity. I'm uncertain whether they
>>     >require attention or if it would be more appropriate to label them as
>>     >false positives within the tool.
>>                                                                                  
>>     >I've submitted this patch in case addressing the issues is necessary.
>>     >However, if reviewers determine that these issues should be marked as
>>     >false positives or ignored within the tool, that option is also
>>     >available
>>                                                                                  
>>     > drivers/gpu/drm/xe/xe_migrate.c | 8 ++++----
>>     > 1 file changed, 4 insertions(+), 4 deletions(-)
>>                                                                                  
>>     >diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>>     >index ee1bb938c493..2ba4fb9511f6 100644
>>     >--- a/drivers/gpu/drm/xe/xe_migrate.c
>>     >+++ b/drivers/gpu/drm/xe/xe_migrate.c
>>     >@@ -227,7 +227,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>>     >                if (vm->flags & XE_VM_FLAG_64K && level == 1)
>>     >                        flags = XE_PDE_64K;
>>     >
>>     >-               entry = vm->pt_ops->pde_encode_bo(bo, map_ofs + (level - 1) *
>>     >+               entry = vm->pt_ops->pde_encode_bo(bo, map_ofs + (u64)(level - 1) *
>>     >                                                  XE_PAGE_SIZE, pat_index);
>>     >                xe_map_wr(xe, &bo->vmap, map_ofs + XE_PAGE_SIZE * level, u64,
>>     >                          entry | flags);
>>     >@@ -235,7 +235,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>>     >
>>     >        /* Write PDE's that point to our BO. */
>>     >        for (i = 0; i < num_entries - num_level; i++) {
>>     >-               entry = vm->pt_ops->pde_encode_bo(bo, i * XE_PAGE_SIZE,
>>     >+               entry = vm->pt_ops->pde_encode_bo(bo, (u64)i * XE_PAGE_SIZE,
>>     >                                                  pat_index);
>>     >
>>     >                xe_map_wr(xe, &bo->vmap, map_ofs + XE_PAGE_SIZE +
>>     >@@ -291,7 +291,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>>     > #define VM_SA_UPDATE_UNIT_SIZE         (XE_PAGE_SIZE / NUM_VMUSA_UNIT_PER_PAGE)
>>     > #define NUM_VMUSA_WRITES_PER_UNIT      (VM_SA_UPDATE_UNIT_SIZE / sizeof(u64))
>>     >        drm_suballoc_manager_init(&m->vm_update_sa,
>>     >-                                 (map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) *
>>     >+                                 (size_t)(map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) *
>>     >                                  NUM_VMUSA_UNIT_PER_PAGE, 0);
>>     >
>>     >        m->pt_bo = bo;
>>     >@@ -490,7 +490,7 @@ static void emit_pte(struct xe_migrate *m,
>>     >        struct xe_vm *vm = m->q->vm;
>>     >        u16 pat_index;
>>     >        u32 ptes;
>>     >-       u64 ofs = at_pt * XE_PAGE_SIZE;
>>     >+       u64 ofs = (u64)at_pt * XE_PAGE_SIZE;
>>     >        u64 cur_ofs;
>>     >
>>     >        /* Indirect access needs compression enabled uncached PAT index */
>>     >--
>>     >2.25.1
>>
>> References
>>
>>     Visible links
>>     1.mailto:matthew.auld@intel.com
>>     2.mailto:matthew.brost@intel.com
>>     3.mailto:rodrigo.vivi@intel.com
>>     4.mailto:himal.prasad.ghimiray@intel.com

[-- Attachment #2: Type: text/html, Size: 36178 bytes --]