[PATCH v1 1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()

[PATCH v1 1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()

[Mirsad Todorovac]

The static analyser tool gave the following advice:

./drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c:1266:7-14: WARNING opportunity for kmemdup

 → 1266         tmp = kmalloc(used_size, GFP_KERNEL);
   1267         if (!tmp)
   1268                 return -ENOMEM;
   1269
 → 1270         memcpy(tmp, &host_telemetry->body.error_count, used_size);

Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
Original code works without fault, so this is not a bug fix but proposed improvement.

Link: https://lwn.net/Articles/198928/
Fixes: 84a2947ecc85c ("drm/amdgpu: Implement virt req_ras_err_count")
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: Xinhui Pan <Xinhui.Pan@amd.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Zhigang Luo <Zhigang.Luo@amd.com>
Cc: Victor Skvortsov <victor.skvortsov@amd.com>
Cc: Hawking Zhang <Hawking.Zhang@amd.com>
Cc: Lijo Lazar <lijo.lazar@amd.com>
Cc: Yunxiang Li <Yunxiang.Li@amd.com>
Cc: Jack Xiao <Jack.Xiao@amd.com>
Cc: Vignesh Chander <Vignesh.Chander@amd.com>
Cc: Danijel Slivka <danijel.slivka@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
---
 v1:
	initial version.

 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index c704e9803e11..0af469ec6fcc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -1263,12 +1263,10 @@ static int amdgpu_virt_cache_host_error_counts(struct amdgpu_device *adev,
 	if (used_size > (AMD_SRIOV_RAS_TELEMETRY_SIZE_KB << 10))
 		return 0;
 
-	tmp = kmalloc(used_size, GFP_KERNEL);
+	tmp = kmemdup(&host_telemetry->body.error_count, used_size, GFP_KERNEL);
 	if (!tmp)
 		return -ENOMEM;
 
-	memcpy(tmp, &host_telemetry->body.error_count, used_size);
-
 	if (checksum != amd_sriov_msg_checksum(tmp, used_size, 0, 0))
 		goto out;
 
-- 
2.43.0

[PATCH v1 2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()

[Mirsad Todorovac]

The source static analysis tool gave the following advice:

./fs/xfs/libxfs/xfs_dir2.c:382:15-22: WARNING opportunity for kmemdup

 → 382         args->value = kmalloc(len,
   383                          GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
   384         if (!args->value)
   385                 return -ENOMEM;
   386
 → 387         memcpy(args->value, name, len);
   388         args->valuelen = len;
   389         return -EEXIST;

Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
Original code works without fault, so this is not a bug fix but proposed improvement.

Link: https://lwn.net/Articles/198928/
Fixes: 94a69db2367ef ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
Fixes: 384f3ced07efd ("[XFS] Return case-insensitive match for dentry cache")
Fixes: 2451337dd0439 ("xfs: global error sign conversion")
Cc: Carlos Maiolino <cem@kernel.org>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: Chandan Babu R <chandanbabu@kernel.org>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: linux-xfs@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
---
 v1:
	initial version.

 fs/xfs/libxfs/xfs_dir2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index 202468223bf9..24251e42bdeb 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -379,12 +379,11 @@ xfs_dir_cilookup_result(
 					!(args->op_flags & XFS_DA_OP_CILOOKUP))
 		return -EEXIST;
 
-	args->value = kmalloc(len,
+	args->value = kmemdup(name, len,
 			GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
 	if (!args->value)
 		return -ENOMEM;
 
-	memcpy(args->value, name, len);
 	args->valuelen = len;
 	return -EEXIST;
 }
-- 
2.43.0

[PATCH v1 3/3] btrfs: replace kmalloc() and memcpy() with kmemdup()

[Mirsad Todorovac]

The static analyser tool gave the following advice:

./fs/btrfs/ioctl.c:4987:9-16: WARNING opportunity for kmemdup

   4986                 if (!iov) {
 → 4987                         iov = kmalloc(sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
   4988                         if (!iov) {
   4989                                 unlock_extent(io_tree, start, lockend, &cached_state);
   4990                                 btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
   4991                                 ret = -ENOMEM;
   4992                                 goto out_acct;
   4993                         }
   4994
 → 4995                         memcpy(iov, iovstack, sizeof(struct iovec) * args.iovcnt);
   4996                 }

Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
Original code works without fault, so this is not a bug fix but proposed improvement.

Link: https://lwn.net/Articles/198928/
Fixes: 34310c442e175 ("btrfs: add io_uring command for encoded reads (ENCODED_READ ioctl)")
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
---
 v1:
	initial version.

 fs/btrfs/ioctl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 3af8bb0c8d75..c2f769334d3c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4984,15 +4984,13 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
 		 * undo this.
 		 */
 		if (!iov) {
-			iov = kmalloc(sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
+			iov = kmemdup(iovstack, sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
 			if (!iov) {
 				unlock_extent(io_tree, start, lockend, &cached_state);
 				btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
 				ret = -ENOMEM;
 				goto out_acct;
 			}
-
-			memcpy(iov, iovstack, sizeof(struct iovec) * args.iovcnt);
 		}
 
 		count = min_t(u64, iov_iter_count(&iter), disk_io_size);
-- 
2.43.0

Re: [PATCH v1 1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()

[Lazar, Lijo]

On 12/18/2024 4:28 AM, Mirsad Todorovac wrote:
> The static analyser tool gave the following advice:
> 
> ./drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c:1266:7-14: WARNING opportunity for kmemdup
> 
>  → 1266         tmp = kmalloc(used_size, GFP_KERNEL);
>    1267         if (!tmp)
>    1268                 return -ENOMEM;
>    1269
>  → 1270         memcpy(tmp, &host_telemetry->body.error_count, used_size);
> 
> Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
> Original code works without fault, so this is not a bug fix but proposed improvement.
> 
> Link: https://lwn.net/Articles/198928/
> Fixes: 84a2947ecc85c ("drm/amdgpu: Implement virt req_ras_err_count")
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Zhigang Luo <Zhigang.Luo@amd.com>
> Cc: Victor Skvortsov <victor.skvortsov@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Cc: Lijo Lazar <lijo.lazar@amd.com>
> Cc: Yunxiang Li <Yunxiang.Li@amd.com>
> Cc: Jack Xiao <Jack.Xiao@amd.com>
> Cc: Vignesh Chander <Vignesh.Chander@amd.com>
> Cc: Danijel Slivka <danijel.slivka@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> ---
>  v1:
> 	initial version.
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index c704e9803e11..0af469ec6fcc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -1263,12 +1263,10 @@ static int amdgpu_virt_cache_host_error_counts(struct amdgpu_device *adev,
>  	if (used_size > (AMD_SRIOV_RAS_TELEMETRY_SIZE_KB << 10))
>  		return 0;
>  
> -	tmp = kmalloc(used_size, GFP_KERNEL);
> +	tmp = kmemdup(&host_telemetry->body.error_count, used_size, GFP_KERNEL);
>  	if (!tmp)
>  		return -ENOMEM;
>  
> -	memcpy(tmp, &host_telemetry->body.error_count, used_size);
> -
>  	if (checksum != amd_sriov_msg_checksum(tmp, used_size, 0, 0))
>  		goto out;
>  

Re: [PATCH v1 3/3] btrfs: replace kmalloc() and memcpy() with kmemdup()

[Mark Harmstone]

There's a fix for this already in the for-next branch:
https://github.com/btrfs/linux/commit/1a287050962c6847fa4918d6b2a0f4cee35c6943

On 17/12/24 22:58, Mirsad Todorovac wrote:
> > 
> The static analyser tool gave the following advice:
> 
> ./fs/btrfs/ioctl.c:4987:9-16: WARNING opportunity for kmemdup
> 
>     4986                 if (!iov) {
>   → 4987                         iov = kmalloc(sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
>     4988                         if (!iov) {
>     4989                                 unlock_extent(io_tree, start, lockend, &cached_state);
>     4990                                 btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>     4991                                 ret = -ENOMEM;
>     4992                                 goto out_acct;
>     4993                         }
>     4994
>   → 4995                         memcpy(iov, iovstack, sizeof(struct iovec) * args.iovcnt);
>     4996                 }
> 
> Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
> Original code works without fault, so this is not a bug fix but proposed improvement.
> 
> Link: https://urldefense.com/v3/__https://lwn.net/Articles/198928/__;!!Bt8RZUm9aw!4OVzQmIUbyH-UGdUwMAL582hR4Q-7HN2fn9IpyxeA1T8qrcC8RdBVz4xuL4m35_kksUllAi6OmdbRehcFpwfHw$
> Fixes: 34310c442e175 ("btrfs: add io_uring command for encoded reads (ENCODED_READ ioctl)")
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: David Sterba <dsterba@suse.com>
> Cc: linux-btrfs@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
> ---
>   v1:
> 	initial version.
> 
>   fs/btrfs/ioctl.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 3af8bb0c8d75..c2f769334d3c 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4984,15 +4984,13 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>   		 * undo this.
>   		 */
>   		if (!iov) {
> -			iov = kmalloc(sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
> +			iov = kmemdup(iovstack, sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
>   			if (!iov) {
>   				unlock_extent(io_tree, start, lockend, &cached_state);
>   				btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>   				ret = -ENOMEM;
>   				goto out_acct;
>   			}
> -
> -			memcpy(iov, iovstack, sizeof(struct iovec) * args.iovcnt);
>   		}
>   
>   		count = min_t(u64, iov_iter_count(&iter), disk_io_size);

Re: [PATCH v1 1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()

[Alex Deucher]

Applied.  Thanks!

Alex

On Wed, Dec 18, 2024 at 3:03 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 12/18/2024 4:28 AM, Mirsad Todorovac wrote:
> > The static analyser tool gave the following advice:
> >
> > ./drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c:1266:7-14: WARNING opportunity for kmemdup
> >
> >  → 1266         tmp = kmalloc(used_size, GFP_KERNEL);
> >    1267         if (!tmp)
> >    1268                 return -ENOMEM;
> >    1269
> >  → 1270         memcpy(tmp, &host_telemetry->body.error_count, used_size);
> >
> > Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
> > Original code works without fault, so this is not a bug fix but proposed improvement.
> >
> > Link: https://lwn.net/Articles/198928/
> > Fixes: 84a2947ecc85c ("drm/amdgpu: Implement virt req_ras_err_count")
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > Cc: David Airlie <airlied@gmail.com>
> > Cc: Simona Vetter <simona@ffwll.ch>
> > Cc: Zhigang Luo <Zhigang.Luo@amd.com>
> > Cc: Victor Skvortsov <victor.skvortsov@amd.com>
> > Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> > Cc: Lijo Lazar <lijo.lazar@amd.com>
> > Cc: Yunxiang Li <Yunxiang.Li@amd.com>
> > Cc: Jack Xiao <Jack.Xiao@amd.com>
> > Cc: Vignesh Chander <Vignesh.Chander@amd.com>
> > Cc: Danijel Slivka <danijel.slivka@amd.com>
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
>
> Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
>
> Thanks,
> Lijo
>
> > ---
> >  v1:
> >       initial version.
> >
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > index c704e9803e11..0af469ec6fcc 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> > @@ -1263,12 +1263,10 @@ static int amdgpu_virt_cache_host_error_counts(struct amdgpu_device *adev,
> >       if (used_size > (AMD_SRIOV_RAS_TELEMETRY_SIZE_KB << 10))
> >               return 0;
> >
> > -     tmp = kmalloc(used_size, GFP_KERNEL);
> > +     tmp = kmemdup(&host_telemetry->body.error_count, used_size, GFP_KERNEL);
> >       if (!tmp)
> >               return -ENOMEM;
> >
> > -     memcpy(tmp, &host_telemetry->body.error_count, used_size);
> > -
> >       if (checksum != amd_sriov_msg_checksum(tmp, used_size, 0, 0))
> >               goto out;
> >
>

Re: [PATCH v1 3/3] btrfs: replace kmalloc() and memcpy() with kmemdup()

[Mirsad Todorovac]

Hi,

Yes, I see you have the prior work and I have duplicated your work.

Apology for the inconvenience.

Best regards,
Mirsad Todorovac

On 12/18/24 11:32, Mark Harmstone wrote:
> There's a fix for this already in the for-next branch:
> https://github.com/btrfs/linux/commit/1a287050962c6847fa4918d6b2a0f4cee35c6943
> 
> On 17/12/24 22:58, Mirsad Todorovac wrote:
>>>
>> The static analyser tool gave the following advice:
>>
>> ./fs/btrfs/ioctl.c:4987:9-16: WARNING opportunity for kmemdup
>>
>>     4986                 if (!iov) {
>>   → 4987                         iov = kmalloc(sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
>>     4988                         if (!iov) {
>>     4989                                 unlock_extent(io_tree, start, lockend, &cached_state);
>>     4990                                 btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>>     4991                                 ret = -ENOMEM;
>>     4992                                 goto out_acct;
>>     4993                         }
>>     4994
>>   → 4995                         memcpy(iov, iovstack, sizeof(struct iovec) * args.iovcnt);
>>     4996                 }
>>
>> Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
>> Original code works without fault, so this is not a bug fix but proposed improvement.
>>
>> Link: https://urldefense.com/v3/__https://lwn.net/Articles/198928/__;!!Bt8RZUm9aw!4OVzQmIUbyH-UGdUwMAL582hR4Q-7HN2fn9IpyxeA1T8qrcC8RdBVz4xuL4m35_kksUllAi6OmdbRehcFpwfHw$
>> Fixes: 34310c442e175 ("btrfs: add io_uring command for encoded reads (ENCODED_READ ioctl)")
>> Cc: Chris Mason <clm@fb.com>
>> Cc: Josef Bacik <josef@toxicpanda.com>
>> Cc: David Sterba <dsterba@suse.com>
>> Cc: linux-btrfs@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
>> ---
>>   v1:
>> 	initial version.
>>
>>   fs/btrfs/ioctl.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 3af8bb0c8d75..c2f769334d3c 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -4984,15 +4984,13 @@ static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd, unsigned int issue
>>   		 * undo this.
>>   		 */
>>   		if (!iov) {
>> -			iov = kmalloc(sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
>> +			iov = kmemdup(iovstack, sizeof(struct iovec) * args.iovcnt, GFP_NOFS);
>>   			if (!iov) {
>>   				unlock_extent(io_tree, start, lockend, &cached_state);
>>   				btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>>   				ret = -ENOMEM;
>>   				goto out_acct;
>>   			}
>> -
>> -			memcpy(iov, iovstack, sizeof(struct iovec) * args.iovcnt);
>>   		}
>>   
>>   		count = min_t(u64, iov_iter_count(&iter), disk_io_size);
> 

Re: [PATCH v1 2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()

[Darrick J. Wong]

On Tue, Dec 17, 2024 at 11:58:12PM +0100, Mirsad Todorovac wrote:
> The source static analysis tool gave the following advice:
> 
> ./fs/xfs/libxfs/xfs_dir2.c:382:15-22: WARNING opportunity for kmemdup
> 
>  → 382         args->value = kmalloc(len,
>    383                          GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
>    384         if (!args->value)
>    385                 return -ENOMEM;
>    386
>  → 387         memcpy(args->value, name, len);
>    388         args->valuelen = len;
>    389         return -EEXIST;
> 
> Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
> Original code works without fault, so this is not a bug fix but proposed improvement.

I guess this is all right, but seeing as this code is shared with
userspace ("libxfs"), making this change will just add to the wrappers
that we have to have:

void *kmemdup_noprof(const void *src, size_t len, gfp_t gfp)
{
	void *p;

	p = kmalloc_node_track_caller_noprof(len, gfp, NUMA_NO_NODE, _RET_IP_);
	if (p)
		memcpy(p, src, len);
	return p;
}

Is this sufficiently better?  That's a question for the kernel
maintainer (cem) and the userspace maintainer (andrey, now cc'd).

--D

> Link: https://lwn.net/Articles/198928/
> Fixes: 94a69db2367ef ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
> Fixes: 384f3ced07efd ("[XFS] Return case-insensitive match for dentry cache")
> Fixes: 2451337dd0439 ("xfs: global error sign conversion")
> Cc: Carlos Maiolino <cem@kernel.org>
> Cc: "Darrick J. Wong" <djwong@kernel.org>
> Cc: Chandan Babu R <chandanbabu@kernel.org>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: linux-xfs@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
> ---
>  v1:
> 	initial version.
> 
>  fs/xfs/libxfs/xfs_dir2.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 202468223bf9..24251e42bdeb 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -379,12 +379,11 @@ xfs_dir_cilookup_result(
>  					!(args->op_flags & XFS_DA_OP_CILOOKUP))
>  		return -EEXIST;
>  
> -	args->value = kmalloc(len,
> +	args->value = kmemdup(name, len,
>  			GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
>  	if (!args->value)
>  		return -ENOMEM;
>  
> -	memcpy(args->value, name, len);
>  	args->valuelen = len;
>  	return -EEXIST;
>  }
> -- 
> 2.43.0
> 
> 

Re: [PATCH v1 2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()

[Andrey Albershteyn]

On 2024-12-18 16:35:21, Darrick J. Wong wrote:
> On Tue, Dec 17, 2024 at 11:58:12PM +0100, Mirsad Todorovac wrote:
> > The source static analysis tool gave the following advice:
> > 
> > ./fs/xfs/libxfs/xfs_dir2.c:382:15-22: WARNING opportunity for kmemdup
> > 
> >  → 382         args->value = kmalloc(len,
> >    383                          GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
> >    384         if (!args->value)
> >    385                 return -ENOMEM;
> >    386
> >  → 387         memcpy(args->value, name, len);
> >    388         args->valuelen = len;
> >    389         return -EEXIST;
> > 
> > Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
> > Original code works without fault, so this is not a bug fix but proposed improvement.
> 
> I guess this is all right, but seeing as this code is shared with
> userspace ("libxfs"), making this change will just add to the wrappers
> that we have to have:
> 
> void *kmemdup_noprof(const void *src, size_t len, gfp_t gfp)
> {
> 	void *p;
> 
> 	p = kmalloc_node_track_caller_noprof(len, gfp, NUMA_NO_NODE, _RET_IP_);
> 	if (p)
> 		memcpy(p, src, len);
> 	return p;
> }
> 
> Is this sufficiently better?  That's a question for the kernel
> maintainer (cem) and the userspace maintainer (andrey, now cc'd).
> 
> --D

There's still possibility to set wrong length in args->valuelen,
which I suppose what this change tries to prevent.

But otherwise wrapper looks good to me

> 
> > Link: https://lwn.net/Articles/198928/
> > Fixes: 94a69db2367ef ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
> > Fixes: 384f3ced07efd ("[XFS] Return case-insensitive match for dentry cache")
> > Fixes: 2451337dd0439 ("xfs: global error sign conversion")
> > Cc: Carlos Maiolino <cem@kernel.org>
> > Cc: "Darrick J. Wong" <djwong@kernel.org>
> > Cc: Chandan Babu R <chandanbabu@kernel.org>
> > Cc: Dave Chinner <dchinner@redhat.com>
> > Cc: linux-xfs@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
> > ---
> >  v1:
> > 	initial version.
> > 
> >  fs/xfs/libxfs/xfs_dir2.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> > index 202468223bf9..24251e42bdeb 100644
> > --- a/fs/xfs/libxfs/xfs_dir2.c
> > +++ b/fs/xfs/libxfs/xfs_dir2.c
> > @@ -379,12 +379,11 @@ xfs_dir_cilookup_result(
> >  					!(args->op_flags & XFS_DA_OP_CILOOKUP))
> >  		return -EEXIST;
> >  
> > -	args->value = kmalloc(len,
> > +	args->value = kmemdup(name, len,
> >  			GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
> >  	if (!args->value)
> >  		return -ENOMEM;
> >  
> > -	memcpy(args->value, name, len);
> >  	args->valuelen = len;
> >  	return -EEXIST;
> >  }
> > -- 
> > 2.43.0
> > 
> > 
> 

-- 
- Andrey

Re: [PATCH v1 2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()

[Darrick J. Wong]

On Sun, Dec 29, 2024 at 01:58:19PM +0100, Andrey Albershteyn wrote:
> On 2024-12-18 16:35:21, Darrick J. Wong wrote:
> > On Tue, Dec 17, 2024 at 11:58:12PM +0100, Mirsad Todorovac wrote:
> > > The source static analysis tool gave the following advice:
> > > 
> > > ./fs/xfs/libxfs/xfs_dir2.c:382:15-22: WARNING opportunity for kmemdup
> > > 
> > >  → 382         args->value = kmalloc(len,
> > >    383                          GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
> > >    384         if (!args->value)
> > >    385                 return -ENOMEM;
> > >    386
> > >  → 387         memcpy(args->value, name, len);
> > >    388         args->valuelen = len;
> > >    389         return -EEXIST;
> > > 
> > > Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
> > > Original code works without fault, so this is not a bug fix but proposed improvement.
> > 
> > I guess this is all right, but seeing as this code is shared with
> > userspace ("libxfs"), making this change will just add to the wrappers
> > that we have to have:
> > 
> > void *kmemdup_noprof(const void *src, size_t len, gfp_t gfp)
> > {
> > 	void *p;
> > 
> > 	p = kmalloc_node_track_caller_noprof(len, gfp, NUMA_NO_NODE, _RET_IP_);
> > 	if (p)
> > 		memcpy(p, src, len);
> > 	return p;
> > }
> > 
> > Is this sufficiently better?  That's a question for the kernel
> > maintainer (cem) and the userspace maintainer (andrey, now cc'd).
> > 
> > --D
> 
> There's still possibility to set wrong length in args->valuelen,
> which I suppose what this change tries to prevent.
> 
> But otherwise wrapper looks good to me

Ok, proceed then.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


> > 
> > > Link: https://lwn.net/Articles/198928/
> > > Fixes: 94a69db2367ef ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
> > > Fixes: 384f3ced07efd ("[XFS] Return case-insensitive match for dentry cache")
> > > Fixes: 2451337dd0439 ("xfs: global error sign conversion")
> > > Cc: Carlos Maiolino <cem@kernel.org>
> > > Cc: "Darrick J. Wong" <djwong@kernel.org>
> > > Cc: Chandan Babu R <chandanbabu@kernel.org>
> > > Cc: Dave Chinner <dchinner@redhat.com>
> > > Cc: linux-xfs@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
> > > ---
> > >  v1:
> > > 	initial version.
> > > 
> > >  fs/xfs/libxfs/xfs_dir2.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> > > index 202468223bf9..24251e42bdeb 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2.c
> > > @@ -379,12 +379,11 @@ xfs_dir_cilookup_result(
> > >  					!(args->op_flags & XFS_DA_OP_CILOOKUP))
> > >  		return -EEXIST;
> > >  
> > > -	args->value = kmalloc(len,
> > > +	args->value = kmemdup(name, len,
> > >  			GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
> > >  	if (!args->value)
> > >  		return -ENOMEM;
> > >  
> > > -	memcpy(args->value, name, len);
> > >  	args->valuelen = len;
> > >  	return -EEXIST;
> > >  }
> > > -- 
> > > 2.43.0
> > > 
> > > 
> > 
> 
> -- 
> - Andrey
> 
> 

Re: [PATCH v1 2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()

[Carlos Maiolino]

Hi Mirsad.

Did you send only this patch, or did I miss patch 1 and 3 of the series? I can't
find them anywhere.

Carlos

On Tue, Dec 17, 2024 at 11:58:12PM +0100, Mirsad Todorovac wrote:
> The source static analysis tool gave the following advice:
> 
> ./fs/xfs/libxfs/xfs_dir2.c:382:15-22: WARNING opportunity for kmemdup
> 
>  → 382         args->value = kmalloc(len,
>    383                          GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
>    384         if (!args->value)
>    385                 return -ENOMEM;
>    386
>  → 387         memcpy(args->value, name, len);
>    388         args->valuelen = len;
>    389         return -EEXIST;
> 
> Replacing kmalloc() + memcpy() with kmemdump() doesn't change semantics.
> Original code works without fault, so this is not a bug fix but proposed improvement.
> 
> Link: https://lwn.net/Articles/198928/
> Fixes: 94a69db2367ef ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
> Fixes: 384f3ced07efd ("[XFS] Return case-insensitive match for dentry cache")
> Fixes: 2451337dd0439 ("xfs: global error sign conversion")
> Cc: Carlos Maiolino <cem@kernel.org>
> Cc: "Darrick J. Wong" <djwong@kernel.org>
> Cc: Chandan Babu R <chandanbabu@kernel.org>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: linux-xfs@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Mirsad Todorovac <mtodorovac69@gmail.com>
> ---
>  v1:
> 	initial version.
> 
>  fs/xfs/libxfs/xfs_dir2.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 202468223bf9..24251e42bdeb 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -379,12 +379,11 @@ xfs_dir_cilookup_result(
>  					!(args->op_flags & XFS_DA_OP_CILOOKUP))
>  		return -EEXIST;
>  
> -	args->value = kmalloc(len,
> +	args->value = kmemdup(name, len,
>  			GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
>  	if (!args->value)
>  		return -ENOMEM;
>  
> -	memcpy(args->value, name, len);
>  	args->valuelen = len;
>  	return -EEXIST;
>  }
> -- 
> 2.43.0
> 
> 

Re: [PATCH v1 2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()

[Carlos Maiolino]

On Tue, 17 Dec 2024 23:58:12 +0100, Mirsad Todorovac wrote:
> The source static analysis tool gave the following advice:
> 
> ./fs/xfs/libxfs/xfs_dir2.c:382:15-22: WARNING opportunity for kmemdup
> 
>  → 382         args->value = kmalloc(len,
>    383                          GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_RETRY_MAYFAIL);
>    384         if (!args->value)
>    385                 return -ENOMEM;
>    386
>  → 387         memcpy(args->value, name, len);
>    388         args->valuelen = len;
>    389         return -EEXIST;
> 
> [...]

Applied to for-next, thanks!

[2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()
      commit: 9d9b72472631262b35157f1a650f066c0e11c2bb

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>

Re: [PATCH v1 1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()

[Carlos Maiolino]

On Tue, 17 Dec 2024 23:58:10 +0100, Mirsad Todorovac wrote:
> The static analyser tool gave the following advice:
> 
> ./drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c:1266:7-14: WARNING opportunity for kmemdup
> 
>  → 1266         tmp = kmalloc(used_size, GFP_KERNEL);
>    1267         if (!tmp)
>    1268                 return -ENOMEM;
>    1269
>  → 1270         memcpy(tmp, &host_telemetry->body.error_count, used_size);
> 
> [...]

Applied to for-next, thanks!

[1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()
      (no commit info)
[2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()
      commit: 9d9b72472631262b35157f1a650f066c0e11c2bb
[3/3] btrfs: replace kmalloc() and memcpy() with kmemdup()
      (no commit info)

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>

Re: [PATCH v1 1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()

[Alex Deucher]

On Tue, Jan 14, 2025 at 5:37 AM Carlos Maiolino <cem@kernel.org> wrote:
>
> On Tue, 17 Dec 2024 23:58:10 +0100, Mirsad Todorovac wrote:
> > The static analyser tool gave the following advice:
> >
> > ./drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c:1266:7-14: WARNING opportunity for kmemdup
> >
> >  → 1266         tmp = kmalloc(used_size, GFP_KERNEL);
> >    1267         if (!tmp)
> >    1268                 return -ENOMEM;
> >    1269
> >  → 1270         memcpy(tmp, &host_telemetry->body.error_count, used_size);
> >
> > [...]
>
> Applied to for-next, thanks!

FWIW, I already picked up the amdgpu patch for drm-next on Dec 18th.

Alex

>
> [1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()
>       (no commit info)
> [2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()
>       commit: 9d9b72472631262b35157f1a650f066c0e11c2bb
> [3/3] btrfs: replace kmalloc() and memcpy() with kmemdup()
>       (no commit info)
>
> Best regards,
> --
> Carlos Maiolino <cem@kernel.org>
>

Re: [PATCH v1 1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()

[Carlos Maiolino]

On Tue, Jan 14, 2025 at 09:27:59AM -0500, Alex Deucher wrote:
> On Tue, Jan 14, 2025 at 5:37 AM Carlos Maiolino <cem@kernel.org> wrote:
> >
> > On Tue, 17 Dec 2024 23:58:10 +0100, Mirsad Todorovac wrote:
> > > The static analyser tool gave the following advice:
> > >
> > > ./drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c:1266:7-14: WARNING opportunity for kmemdup
> > >
> > >  → 1266         tmp = kmalloc(used_size, GFP_KERNEL);
> > >    1267         if (!tmp)
> > >    1268                 return -ENOMEM;
> > >    1269
> > >  → 1270         memcpy(tmp, &host_telemetry->body.error_count, used_size);
> > >
> > > [...]
> >
> > Applied to for-next, thanks!
> 
> FWIW, I already picked up the amdgpu patch for drm-next on Dec 18th.
> 

b4 seems to have ignored that I pulled only the xfs bits :)

Thanks for the heads up Alex.

> Alex
> 
> >
> > [1/3] drm/admgpu: replace kmalloc() and memcpy() with kmemdup()
> >       (no commit info)
> > [2/3] xfs/libxfs: replace kmalloc() and memcpy() with kmemdup()
> >       commit: 9d9b72472631262b35157f1a650f066c0e11c2bb
> > [3/3] btrfs: replace kmalloc() and memcpy() with kmemdup()
> >       (no commit info)
> >
> > Best regards,
> > --
> > Carlos Maiolino <cem@kernel.org>
> >