Re: [PATCH] drm/xe: Add NULL check before deferencing pointer

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 19 Aug 2024, apoorva.singh@intel.com wrote:
> From: Apoorva Singh <apoorva.singh@intel.com>

*dereferencing typo in the subject. You could also be more specific,
that subject could refer to anything anywhere in the driver. If you find
another one, what's its subject going to be?

> Add NULL check before deferencing of lrc->bo in
> xe_lrc_snapshot_capture().
>
> Free the dynamically allocated memory for snapshot
> to avoid memory leak

We've seen this patch or its variants many times now. Where's the
changelog? What's going on?

> Signed-off-by: Apoorva Singh <apoorva.singh@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_lrc.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> index 974a9cd8c379..e31d9bc64ff6 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -1652,20 +1652,24 @@ struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
>  	if (lrc->bo && lrc->bo->vm)
>  		xe_vm_get(lrc->bo->vm);
>  
> -	snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
> -	snapshot->indirect_context_desc = xe_lrc_indirect_ring_ggtt_addr(lrc);
> -	snapshot->head = xe_lrc_ring_head(lrc);
> -	snapshot->tail.internal = lrc->ring.tail;
> -	snapshot->tail.memory = xe_lrc_ring_tail(lrc);
> -	snapshot->start_seqno = xe_lrc_start_seqno(lrc);
> -	snapshot->seqno = xe_lrc_seqno(lrc);
> -	snapshot->lrc_bo = xe_bo_get(lrc->bo);
> -	snapshot->lrc_offset = xe_lrc_pphwsp_offset(lrc);
> -	snapshot->lrc_size = lrc->bo->size - snapshot->lrc_offset;
> -	snapshot->lrc_snapshot = NULL;
> -	snapshot->ctx_timestamp = xe_lrc_ctx_timestamp(lrc);
> -	snapshot->ctx_job_timestamp = xe_lrc_ctx_job_timestamp(lrc);
> -	return snapshot;
> +	if (lrc->bo) {

Please handle error cases in if branches with early returns, and do the
happy day scenario in the top indentation level.


BR,
Jani.


> +		snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
> +		snapshot->indirect_context_desc = xe_lrc_indirect_ring_ggtt_addr(lrc);
> +		snapshot->head = xe_lrc_ring_head(lrc);
> +		snapshot->tail.internal = lrc->ring.tail;
> +		snapshot->tail.memory = xe_lrc_ring_tail(lrc);
> +		snapshot->start_seqno = xe_lrc_start_seqno(lrc);
> +		snapshot->seqno = xe_lrc_seqno(lrc);
> +		snapshot->lrc_bo = xe_bo_get(lrc->bo);
> +		snapshot->lrc_offset = xe_lrc_pphwsp_offset(lrc);
> +		snapshot->lrc_size = lrc->bo->size - snapshot->lrc_offset;
> +		snapshot->lrc_snapshot = NULL;
> +		snapshot->ctx_timestamp = xe_lrc_ctx_timestamp(lrc);
> +		snapshot->ctx_job_timestamp = xe_lrc_ctx_job_timestamp(lrc);
> +		return snapshot;
> +	}
> +	kfree(snapshot);
> +	return NULL;
>  }
>  
>  void xe_lrc_snapshot_capture_delayed(struct xe_lrc_snapshot *snapshot)

-- 
Jani Nikula, Intel