Re: [PATCH] amdkfd: initialize svm lists at where they are defined

[Felix Kuehling <felix.kuehling@amd.com>]

On 2025-02-21 4:23, Zhu Lingshan wrote:
> This commit initialized svm lists at where they are
> defined. This is defensive programing for security
> and consistency.
>
> Initalizing variables ensures that their states are
> always valid, avoiding issues caused by
> uninitialized variables that could lead to
> unpredictable behaviros.

The lists are clearly documented as output parameters in the svm_range_add function definition. I think it's more readable to do the list initialization in svm_range_add to keep the logic of the caller more readable. One suggestion inline that would move the initialization to the caller without cluttering the calling function's code.


>
> And we should not assume the callee would always
> initialize them
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index bd3e20d981e0..cbc997449379 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2130,11 +2130,7 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>  
>  	pr_debug("svms 0x%p [0x%llx 0x%lx]\n", &p->svms, start, last);
>  
> -	INIT_LIST_HEAD(update_list);
> -	INIT_LIST_HEAD(insert_list);
> -	INIT_LIST_HEAD(remove_list);
>  	INIT_LIST_HEAD(&new_list);
> -	INIT_LIST_HEAD(remap_list);
>  
>  	node = interval_tree_iter_first(&svms->objects, start, last);
>  	while (node) {
> @@ -3635,6 +3631,11 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>  	if (r)
>  		return r;
>  
> +	INIT_LIST_HEAD(&update_list);
> +	INIT_LIST_HEAD(&insert_list);
> +	INIT_LIST_HEAD(&remove_list);
> +	INIT_LIST_HEAD(&remap_list);
> +

You could initialize these where they are defined by replacing the struct list_head ... definitions with

	LIST_HEAD(update_list);
	LIST_HEAD(insert_list);
	...

Regards,
  Felix


>  	svms = &p->svms;
>  
>  	mutex_lock(&process_info->lock);