Re: [PATCH v2 1/1] drm/xe: Prevent null pointer access in xe_migrate_copy

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 20, 2024 at 02:44:56PM -0700, Matt Roper wrote:
> On Fri, Sep 20, 2024 at 09:58:14AM +0530, Ghimiray, Himal Prasad wrote:
> > 
> > 
> > On 20-09-2024 04:12, Zhanjun Dong wrote:
> > > Update lacks source flag to include resource is null case. This will
> > > prevent null pointer derefrence in xe_migrate_copy.
> > > 
> > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_bo.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > > index 5f2f1ec46b57..5e8f60a8d431 100644
> > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > @@ -682,8 +682,8 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> > >   	tt_has_data = ttm && (ttm_tt_is_populated(ttm) ||
> > >   			      (ttm->page_flags & TTM_TT_FLAG_SWAPPED));
> > > -	move_lacks_source = handle_system_ccs ? (!bo->ccs_cleared)  :
> > > -						(!mem_type_is_vram(old_mem_type) && !tt_has_data);
> > > +	move_lacks_source = !old_mem || (handle_system_ccs ? (!bo->ccs_cleared) :
> > > +					 (!mem_type_is_vram(old_mem_type) && !tt_has_data));
> > 
> > 
> > Just for curiosity, isn't !old_mem implicitly taken care here ?
> > shouldn't ttm be NULL, if resource is NULL ? IIRC, this was what Thomas had
> > confirmed during handle_system_ccs implementation.

This was my original comment too. 

But I guess we do have a this:

 673         /* Bo creation path, moving to system or TT. */
 674         if ((!old_mem && ttm) && !handle_system_ccs) {

Which seems to indicates !old_mem && ttm can be possible. So I think the
patch is actually correct? 

> 
> Drive-by comment:  If this is an invariant, it might still be worth
> adding an xe_assert() so that CI can ensure the condition never gets
> violated by future code refactors and design changes.
> 
> 
> Matt
> 
> > 
> > Thomas/Matt,
> > Can you confirm here ?
> >

I'd have to dig into TTM a bit more to really understand what is going
on here. Thomas might just know how !old_mem && ttm can evalulate to
true.

Matt

> > BR
> > Himal
> > 
> > 
> > >   	needs_clear = (ttm && ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC) ||
> > >   		(!ttm && ttm_bo->type == ttm_bo_type_device);
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation