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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Sat, 2024-09-21 at 00:24 +0000, Matthew Brost wrote:
> 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.

Please add relevant parts of the NULL pointer trace to the commit
message and add a Fixes: tag for the commit that caused the bug. Also
please describe how this patch fixes the bug.

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

I think in that case I might have been wrong.
If we're creating a PL_TT bo then IIRC bo->ttm is non-NULL and old_mem
== NULL.

Thanks,
Thomas



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