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

[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

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>