linux-kernel - Re: [PATCH v2] drm/amdkfd: Change svm_range_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e3da6807-3d39-43b6-89af-a67c5e231300@amd.com>
Date: Mon, 31 Mar 2025 18:52:27 -0400
From: Felix Kuehling <felix.kuehling@....com>
To: Ваторопин Андрей
 <a.vatoropin@...t.ru>
Cc: Alex Deucher <alexander.deucher@....com>,
 Christian König <christian.koenig@....com>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>
Subject: Re: [PATCH v2] drm/amdkfd: Change svm_range_get_info return type

On 2025-03-31 09:18, Ваторопин Андрей wrote:
> From: Andrey Vatoropin <a.vatoropin@...t.ru>
>
> Static analysis shows that pointer "svms" cannot be NULL because it points
> to the object "struct svm_range_list". Remove the extra NULL check. It is
> meaningless and harms the readability of the code.
>
> In the function svm_range_get_info() there is no possibility of failure.
> Therefore, the caller of the function svm_range_get_info() does not need
> a return value. Change the function svm_range_get_info() return type from
> "int" to "void".
>
> Since the function svm_range_get_info() has a return type of "void". The
> caller of the function svm_range_get_info() does not need a return value.
> Delete extra code.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Andrey Vatoropin <a.vatoropin@...t.ru>
> ---
> v1 -> v2: also change return type of svm_range_get_info() per Felix Kuehling suggestion
Thank you for the patch. It seems you lost the change in 
kfd_criu_checkpoint_svm from the first version. Was that accidental or 
were you planning to send another patch?

I'm also having trouble applying your patches automatically with git am. 
Apparently you're using DOS CR-LF line endings, and --no-keep-cr isn't 
helping. I finally made it work with "git am --quoted-cr=strip ...". 
Please check your workflow for generating patches.

Thanks,
   Felix


>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c |  4 +---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c     |  7 ++-----
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.h     | 11 +++++------
>   3 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 1e9dd00620bf..a2149afa5803 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -2039,9 +2039,7 @@ static int criu_get_process_object_info(struct kfd_process *p,
>   
>   	num_events = kfd_get_num_events(p);
>   
> -	ret = svm_range_get_info(p, &num_svm_ranges, &svm_priv_data_size);
> -	if (ret)
> -		return ret;
> +	svm_range_get_info(p, &num_svm_ranges, &svm_priv_data_size);
>   
>   	*num_objects = num_queues + num_events + num_svm_ranges;
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 100717a98ec1..1b7d57a1b245 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -4076,8 +4076,8 @@ int kfd_criu_restore_svm(struct kfd_process *p,
>   	return ret;
>   }
>   
> -int svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
> -		       uint64_t *svm_priv_data_size)
> +void svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
> +			uint64_t *svm_priv_data_size)
>   {
>   	uint64_t total_size, accessibility_size, common_attr_size;
>   	int nattr_common = 4, nattr_accessibility = 1;
> @@ -4089,8 +4089,6 @@ int svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
>   	*svm_priv_data_size = 0;
>   
>   	svms = &p->svms;
> -	if (!svms)
> -		return -EINVAL;
>   
>   	mutex_lock(&svms->lock);
>   	list_for_each_entry(prange, &svms->list, list) {
> @@ -4132,7 +4130,6 @@ int svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
>   
>   	pr_debug("num_svm_ranges %u total_priv_size %llu\n", *num_svm_ranges,
>   		 *svm_priv_data_size);
> -	return 0;
>   }
>   
>   int kfd_criu_checkpoint_svm(struct kfd_process *p,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> index 6ea23c78009c..01c7a4877904 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> @@ -184,8 +184,8 @@ void schedule_deferred_list_work(struct svm_range_list *svms);
>   void svm_range_dma_unmap_dev(struct device *dev, dma_addr_t *dma_addr,
>   			 unsigned long offset, unsigned long npages);
>   void svm_range_dma_unmap(struct svm_range *prange);
> -int svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
> -		       uint64_t *svm_priv_data_size);
> +void svm_range_get_info(struct kfd_process *p, uint32_t *num_svm_ranges,
> +			uint64_t *svm_priv_data_size);
>   int kfd_criu_checkpoint_svm(struct kfd_process *p,
>   			    uint8_t __user *user_priv_data,
>   			    uint64_t *priv_offset);
> @@ -237,13 +237,12 @@ static inline int svm_range_schedule_evict_svm_bo(
>   	return -EINVAL;
>   }
>   
> -static inline int svm_range_get_info(struct kfd_process *p,
> -				     uint32_t *num_svm_ranges,
> -				     uint64_t *svm_priv_data_size)
> +static inline void svm_range_get_info(struct kfd_process *p,
> +				      uint32_t *num_svm_ranges,
> +				      uint64_t *svm_priv_data_size)
>   {
>   	*num_svm_ranges = 0;
>   	*svm_priv_data_size = 0;
> -	return 0;
>   }
>   
>   static inline int kfd_criu_checkpoint_svm(struct kfd_process *p,