[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Kuehling, Felix <[email protected]> > Sent: Wednesday, December 4, 2024 7:01 PM > To: Russell, Kent <[email protected]>; Martin, Andrew > <[email protected]>; [email protected] > Cc: Tudor, Alexandru <[email protected]> > Subject: Re: [PATCH v2] drm/amdkfd: Dereference null return value > > > On 2024-12-03 09:30, Russell, Kent wrote: > > > > [Public] > > > > > > > > > > ---------------------------------------------------------------------- > > -- > > *From:* amd-gfx <[email protected]> on behalf of > > Andrew Martin <[email protected]> > > *Sent:* Monday, December 2, 2024 7:45:55 a.m. > > *To:* [email protected] <[email protected]> > > *Cc:* Kuehling, Felix <[email protected]>; Tudor, Alexandru > > <[email protected]>; Martin, Andrew > <[email protected]>; > > Martin, Andrew <[email protected]> > > *Subject:* [PATCH v2] drm/amdkfd: Dereference null return value > > > > In the function pqm_uninit there is a call-assignment of "pdd = > > kfd_get_process_device_data" which could be null, and this value was > > later dereferenced without checking. > > > > Fixes: fb91065851cd ("drm/amdkfd: Refactor queue wptr_bo GART > > mapping") > > Signed-off-by: Andrew Martin <[email protected]> > > --- > > drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > index c76db22a1000..89aa578f6c68 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > @@ -212,11 +212,13 @@ static void pqm_clean_queue_resource(struct > > process_queue_manager *pqm, > > void pqm_uninit(struct process_queue_manager *pqm) > > { > > struct process_queue_node *pqn, *next; > > - struct kfd_process_device *pdd; > > > > list_for_each_entry_safe(pqn, next, &pqm->queues, > > process_queue_list) { > > if (pqn->q) { > > - pdd = > > kfd_get_process_device_data(pqn->q->device, pqm->process); > > + struct kfd_process_device *pdd = > > kfd_get_process_device_data(pqn->q->device, > > + pqm->process); > > + if (WARN_ON(!pdd)) > > > > Would we want a "continue" instead of "break" if the pdd is NULL? Just > > in case other ones in the list are still valid? Or is one NULL enough > > to just WARN and abort? > > I agree, we should use "continue" here. We are leaking memory, but let's not > leak more than necessary. With that fixed, the patch is > Greetings Kent/Felix, sending peace.
Thanks, I will make the change, run the tests and send it out. > Reviewed-by: Felix Kuehling <[email protected]> > > > Thanks, > Felix > > > > > > Kent > > > > + return; > > kfd_queue_unref_bo_vas(pdd, > > &pqn->q->properties); > > kfd_queue_release_buffers(pdd, > > &pqn->q->properties); > > pqm_clean_queue_resource(pqm, pqn); > > -- > > 2.43.0 > > > >
