diff mbox series

mm/util: Swap kmemdup_array() arguments

Message ID 20240606144608.97817-2-jean-philippe@linaro.org (mailing list archive)
State New
Headers show
Series mm/util: Swap kmemdup_array() arguments | expand

Commit Message

Jean-Philippe Brucker June 6, 2024, 2:46 p.m. UTC 
GCC 14.1 complains about the argument usage of kmemdup_array():

  drivers/soc/tegra/fuse/fuse-tegra.c:130:65: error: 'kmemdup_array' sizes specified with 'sizeof' in the earlier argument and not in the later argument [-Werror=calloc-transposed-args]
    130 |         fuse->lookups = kmemdup_array(fuse->soc->lookups, sizeof(*fuse->lookups),
        |                                                                 ^
  drivers/soc/tegra/fuse/fuse-tegra.c:130:65: note: earlier argument should specify number of elements, later size of each element

The annotation introduced by commit 7d78a7773355 ("string: Add
additional __realloc_size() annotations for "dup" helpers") lets the
compiler think that kmemdup_array() follows the same format as calloc(),
with the number of elements preceding the size of one element. So we
could simply swap the arguments to __realloc_size() to get rid of that
warning, but it seems cleaner to instead have kmemdup_array() follow the
same format as krealloc_array(), memdup_array_user(), calloc() etc.

Fixes: 7d78a7773355 ("string: Add additional __realloc_size() annotations for "dup" helpers")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 include/linux/string.h              | 2 +-
 drivers/soc/tegra/fuse/fuse-tegra.c | 4 ++--
 lib/fortify_kunit.c                 | 2 +-
 mm/util.c                           | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)


base-commit: 2df0193e62cf887f373995fb8a91068562784adc

Comments

Andy Shevchenko June 6, 2024, 3:36 p.m. UTC | #1 
On Thu, Jun 6, 2024 at 5:47 PM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> GCC 14.1 complains about the argument usage of kmemdup_array():
>
>   drivers/soc/tegra/fuse/fuse-tegra.c:130:65: error: 'kmemdup_array' sizes specified with 'sizeof' in the earlier argument and not in the later argument [-Werror=calloc-transposed-args]
>     130 |         fuse->lookups = kmemdup_array(fuse->soc->lookups, sizeof(*fuse->lookups),
>         |                                                                 ^
>   drivers/soc/tegra/fuse/fuse-tegra.c:130:65: note: earlier argument should specify number of elements, later size of each element
>
> The annotation introduced by commit 7d78a7773355 ("string: Add
> additional __realloc_size() annotations for "dup" helpers") lets the
> compiler think that kmemdup_array() follows the same format as calloc(),
> with the number of elements preceding the size of one element. So we
> could simply swap the arguments to __realloc_size() to get rid of that
> warning, but it seems cleaner to instead have kmemdup_array() follow the
> same format as krealloc_array(), memdup_array_user(), calloc() etc.

LGTM, it seems we don't have many users (yet), this looks like a
comprehensive change.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Kees Cook June 6, 2024, 3:55 p.m. UTC | #2 
On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote:
> GCC 14.1 complains about the argument usage of kmemdup_array():
> 
>   drivers/soc/tegra/fuse/fuse-tegra.c:130:65: error: 'kmemdup_array' sizes specified with 'sizeof' in the earlier argument and not in the later argument [-Werror=calloc-transposed-args]
>     130 |         fuse->lookups = kmemdup_array(fuse->soc->lookups, sizeof(*fuse->lookups),
>         |                                                                 ^
>   drivers/soc/tegra/fuse/fuse-tegra.c:130:65: note: earlier argument should specify number of elements, later size of each element
> 
> [...]

Applied to for-next/hardening, thanks!

[1/1] mm/util: Swap kmemdup_array() arguments
      https://git.kernel.org/kees/c/0ee14725471c

Take care,
Kees Cook June 6, 2024, 5:46 p.m. UTC | #3 
On Thu, Jun 06, 2024 at 08:35:13PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 6, 2024 at 6:56 PM Kees Cook <kees@kernel.org> wrote:
> > On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote:
> 
> [...]
> 
> > Applied to for-next/hardening, thanks!
> 
> Btw, is it possible to get this for v6.10, so we may start enabling it
> for others?

Which others do you mean?
Andy Shevchenko June 6, 2024, 5:48 p.m. UTC | #4 
On Thu, Jun 6, 2024 at 8:46 PM Kees Cook <kees@kernel.org> wrote:
>
> On Thu, Jun 06, 2024 at 08:35:13PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 6, 2024 at 6:56 PM Kees Cook <kees@kernel.org> wrote:
> > > On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote:
> >
> > [...]
> >
> > > Applied to for-next/hardening, thanks!
> >
> > Btw, is it possible to get this for v6.10, so we may start enabling it
> > for others?
>
> Which others do you mean?

There are a lot of users of kmemdup(x*y) which I want to convert
sooner than later to kmemdup_array(x,y).

FWIW, this patch even has a Fixes tag!
Kees Cook June 6, 2024, 6:10 p.m. UTC | #5 
On Thu, Jun 06, 2024 at 08:48:37PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 6, 2024 at 8:46 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Thu, Jun 06, 2024 at 08:35:13PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 6, 2024 at 6:56 PM Kees Cook <kees@kernel.org> wrote:
> > > > On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote:
> > >
> > > [...]
> > >
> > > > Applied to for-next/hardening, thanks!
> > >
> > > Btw, is it possible to get this for v6.10, so we may start enabling it
> > > for others?
> >
> > Which others do you mean?
> 
> There are a lot of users of kmemdup(x*y) which I want to convert
> sooner than later to kmemdup_array(x,y).

Ah-ha, I see what you mean. Well, I'm not sure we can do v6.10 for this
because rc2 is behind us, and that's what most subsystems merge to. I
can land the patch for rc3 so there will be no warnings in Linus's
tree/-next, but conversions in subsystem trees will gain warnings, I
think...
Andy Shevchenko June 6, 2024, 6:45 p.m. UTC | #6 
On Thu, Jun 06, 2024 at 11:10:43AM -0700, Kees Cook wrote:
> On Thu, Jun 06, 2024 at 08:48:37PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 6, 2024 at 8:46 PM Kees Cook <kees@kernel.org> wrote:
> > >
> > > On Thu, Jun 06, 2024 at 08:35:13PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jun 6, 2024 at 6:56 PM Kees Cook <kees@kernel.org> wrote:
> > > > > On Thu, 06 Jun 2024 15:46:09 +0100, Jean-Philippe Brucker wrote:
> > > >
> > > > [...]
> > > >
> > > > > Applied to for-next/hardening, thanks!
> > > >
> > > > Btw, is it possible to get this for v6.10, so we may start enabling it
> > > > for others?
> > >
> > > Which others do you mean?
> > 
> > There are a lot of users of kmemdup(x*y) which I want to convert
> > sooner than later to kmemdup_array(x,y).
> 
> Ah-ha, I see what you mean. Well, I'm not sure we can do v6.10 for this
> because rc2 is behind us, and that's what most subsystems merge to. I
> can land the patch for rc3 so there will be no warnings in Linus's
> tree/-next, but conversions in subsystem trees will gain warnings, I
> think...

I see, but v6.10-rc3 is still better than v6.11-rc1. Some of the subsystems are
okay to merge rcX to their for-next if needed.
diff mbox series

Patch

diff --git a/include/linux/string.h b/include/linux/string.h
index 60168aa2af075..9edace076ddbf 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -289,7 +289,7 @@  extern void *kmemdup_noprof(const void *src, size_t len, gfp_t gfp) __realloc_si
 
 extern void *kvmemdup(const void *src, size_t len, gfp_t gfp) __realloc_size(2);
 extern char *kmemdup_nul(const char *s, size_t len, gfp_t gfp);
-extern void *kmemdup_array(const void *src, size_t element_size, size_t count, gfp_t gfp)
+extern void *kmemdup_array(const void *src, size_t count, size_t element_size, gfp_t gfp)
 		__realloc_size(2, 3);
 
 /* lib/argv_split.c */
diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index b6bfd6729df39..d276672838465 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -127,8 +127,8 @@  static void tegra_fuse_print_sku_info(struct tegra_sku_info *tegra_sku_info)
 
 static int tegra_fuse_add_lookups(struct tegra_fuse *fuse)
 {
-	fuse->lookups = kmemdup_array(fuse->soc->lookups, sizeof(*fuse->lookups),
-				      fuse->soc->num_lookups, GFP_KERNEL);
+	fuse->lookups = kmemdup_array(fuse->soc->lookups, fuse->soc->num_lookups,
+				      sizeof(*fuse->lookups), GFP_KERNEL);
 	if (!fuse->lookups)
 		return -ENOMEM;
 
diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c
index f9cc467334ce3..e17d520f532cf 100644
--- a/lib/fortify_kunit.c
+++ b/lib/fortify_kunit.c
@@ -374,7 +374,7 @@  static const char * const test_strs[] = {
 	for (i = 0; i < ARRAY_SIZE(test_strs); i++) {			\
 		len = strlen(test_strs[i]);				\
 		KUNIT_EXPECT_EQ(test, __builtin_constant_p(len), 0);	\
-		checker(len, kmemdup_array(test_strs[i], len, 1, gfp),	\
+		checker(len, kmemdup_array(test_strs[i], 1, len, gfp),	\
 			kfree(p));					\
 		checker(len, kmemdup(test_strs[i], len, gfp),		\
 			kfree(p));					\
diff --git a/mm/util.c b/mm/util.c
index c9e519e6811f5..6682097372efc 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -139,14 +139,14 @@  EXPORT_SYMBOL(kmemdup_noprof);
  * kmemdup_array - duplicate a given array.
  *
  * @src: array to duplicate.
- * @element_size: size of each element of array.
  * @count: number of elements to duplicate from array.
+ * @element_size: size of each element of array.
  * @gfp: GFP mask to use.
  *
  * Return: duplicated array of @src or %NULL in case of error,
  * result is physically contiguous. Use kfree() to free.
  */
-void *kmemdup_array(const void *src, size_t element_size, size_t count, gfp_t gfp)
+void *kmemdup_array(const void *src, size_t count, size_t element_size, gfp_t gfp)
 {
 	return kmemdup(src, size_mul(element_size, count), gfp);
 }