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@xxxxxxxxxx>
> Cc: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> Cc: Chandan Babu R <chandanbabu@xxxxxxxxxx>
> Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> Cc: linux-xfs@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Mirsad Todorovac <mtodorovac69@xxxxxxxxx>
> ---
> 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
>
>
