[PATCH v1 0/5] Introduce devm_kmemdup_array() helper

[PATCH v1 0/5] Introduce devm_kmemdup_array() helper

[Raag Jadav]

This series introduces devm_kmemdup_array() helper with multiplication
overflow check and uses it across drivers.

Raag Jadav (5):
  devres: Introduce devm_kmemdup_array()
  pinctrl: intel: copy communities using devm_kmemdup_array()
  pinctrl: pxa2xx: use devm_kmemdup_array()
  input: sparse-keymap: use devm_kmemdup_array()
  ASoC: Intel: avs: use devm_kmemdup_array()

 drivers/input/sparse-keymap.c         |  3 +--
 drivers/pinctrl/intel/pinctrl-intel.c |  6 ++----
 drivers/pinctrl/pxa/pinctrl-pxa2xx.c  |  8 ++++----
 include/linux/device.h                | 10 ++++++++++
 sound/soc/intel/avs/boards/da7219.c   |  3 ++-
 sound/soc/intel/avs/boards/es8336.c   |  3 ++-
 sound/soc/intel/avs/boards/nau8825.c  |  3 ++-
 sound/soc/intel/avs/boards/rt274.c    |  3 ++-
 sound/soc/intel/avs/boards/rt286.c    |  3 ++-
 sound/soc/intel/avs/boards/rt298.c    |  3 ++-
 sound/soc/intel/avs/boards/rt5663.c   |  3 ++-
 sound/soc/intel/avs/boards/rt5682.c   |  2 +-
 12 files changed, 32 insertions(+), 18 deletions(-)

-- 
2.35.3

[PATCH v1 1/5] devres: Introduce devm_kmemdup_array()

[Raag Jadav]

Introduce '_array' variant of devm_kmemdup() for the users which lack
multiplication overflow check.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 include/linux/device.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index b4bde8d22697..c31f48d0dde0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -358,6 +358,16 @@ char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
 const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);
 void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
 	__realloc_size(3);
+static inline void *devm_kmemdup_array(struct device *dev, const void *src,
+				       size_t n, size_t size, gfp_t flags)
+{
+	size_t bytes;
+
+	if (unlikely(check_mul_overflow(n, size, &bytes)))
+		return NULL;
+
+	return devm_kmemdup(dev, src, bytes, flags);
+}
 
 unsigned long devm_get_free_pages(struct device *dev,
 				  gfp_t gfp_mask, unsigned int order);
-- 
2.35.3

[PATCH v1 2/5] pinctrl: intel: copy communities using devm_kmemdup_array()

[Raag Jadav]

Copy communities using devm_kmemdup_array() instead of doing it manually.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 04b438f63ccb..e165bd5b5811 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1577,8 +1577,8 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 	 * to the registers.
 	 */
 	pctrl->ncommunities = pctrl->soc->ncommunities;
-	pctrl->communities = devm_kcalloc(dev, pctrl->ncommunities,
-					  sizeof(*pctrl->communities), GFP_KERNEL);
+	pctrl->communities = devm_kmemdup_array(dev, pctrl->soc->communities, pctrl->ncommunities,
+						sizeof(*pctrl->communities), GFP_KERNEL);
 	if (!pctrl->communities)
 		return -ENOMEM;
 
@@ -1588,8 +1588,6 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 		u32 offset;
 		u32 value;
 
-		*community = pctrl->soc->communities[i];
-
 		regs = devm_platform_ioremap_resource(pdev, community->barno);
 		if (IS_ERR(regs))
 			return PTR_ERR(regs);
-- 
2.35.3

[PATCH v1 3/5] pinctrl: pxa2xx: use devm_kmemdup_array()

[Raag Jadav]

Convert to use devm_kmemdup_array() which is more robust.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/pinctrl/pxa/pinctrl-pxa2xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
index 9e34b92ff5f2..9fd7a8fb2bc4 100644
--- a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
+++ b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
@@ -281,9 +281,8 @@ static int pxa2xx_build_functions(struct pxa_pinctrl *pctl)
 		for (df = pctl->ppins[i].functions; df->name; df++)
 			if (!pxa2xx_find_function(pctl, df->name, functions))
 				(functions + pctl->nfuncs++)->name = df->name;
-	pctl->functions = devm_kmemdup(pctl->dev, functions,
-				       pctl->nfuncs * sizeof(*functions),
-				       GFP_KERNEL);
+	pctl->functions = devm_kmemdup_array(pctl->dev, functions, pctl->nfuncs,
+					     sizeof(*functions), GFP_KERNEL);
 	if (!pctl->functions)
 		return -ENOMEM;
 
@@ -314,7 +313,8 @@ static int pxa2xx_build_groups(struct pxa_pinctrl *pctl)
 						pctl->ppins[j].pin.name;
 		func = pctl->functions + i;
 		func->ngroups = ngroups;
-		func->groups = devm_kmemdup(pctl->dev, gtmp, ngroups * sizeof(*gtmp), GFP_KERNEL);
+		func->groups = devm_kmemdup_array(pctl->dev, gtmp, ngroups,
+						  sizeof(*gtmp), GFP_KERNEL);
 		if (!func->groups)
 			return -ENOMEM;
 	}
-- 
2.35.3

[PATCH v1 4/5] input: sparse-keymap: use devm_kmemdup_array()

[Raag Jadav]

Convert to use devm_kmemdup_array() which is more robust.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/input/sparse-keymap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c
index 25bf8be6e711..5ec3b9ebcac5 100644
--- a/drivers/input/sparse-keymap.c
+++ b/drivers/input/sparse-keymap.c
@@ -176,8 +176,7 @@ int sparse_keymap_setup(struct input_dev *dev,
 	for (e = keymap; e->type != KE_END; e++)
 		map_size++;
 
-	map = devm_kmemdup(&dev->dev, keymap, map_size * sizeof(*map),
-			   GFP_KERNEL);
+	map = devm_kmemdup_array(&dev->dev, keymap, map_size, sizeof(*map), GFP_KERNEL);
 	if (!map)
 		return -ENOMEM;
 
-- 
2.35.3

[PATCH v1 5/5] ASoC: Intel: avs: use devm_kmemdup_array()

[Raag Jadav]

Convert to use devm_kmemdup_array() which is more robust.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 sound/soc/intel/avs/boards/da7219.c  | 3 ++-
 sound/soc/intel/avs/boards/es8336.c  | 3 ++-
 sound/soc/intel/avs/boards/nau8825.c | 3 ++-
 sound/soc/intel/avs/boards/rt274.c   | 3 ++-
 sound/soc/intel/avs/boards/rt286.c   | 3 ++-
 sound/soc/intel/avs/boards/rt298.c   | 3 ++-
 sound/soc/intel/avs/boards/rt5663.c  | 3 ++-
 sound/soc/intel/avs/boards/rt5682.c  | 2 +-
 8 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/sound/soc/intel/avs/boards/da7219.c b/sound/soc/intel/avs/boards/da7219.c
index 80c0a1a95654..1b8f58b611a4 100644
--- a/sound/soc/intel/avs/boards/da7219.c
+++ b/sound/soc/intel/avs/boards/da7219.c
@@ -113,7 +113,8 @@ static int avs_da7219_codec_init(struct snd_soc_pcm_runtime *runtime)
 	}
 
 	num_pins = ARRAY_SIZE(card_headset_pins);
-	pins = devm_kmemdup(card->dev, card_headset_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_headset_pins, num_pins,
+				  sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
diff --git a/sound/soc/intel/avs/boards/es8336.c b/sound/soc/intel/avs/boards/es8336.c
index c8522e2430f8..8103e539e08a 100644
--- a/sound/soc/intel/avs/boards/es8336.c
+++ b/sound/soc/intel/avs/boards/es8336.c
@@ -109,7 +109,8 @@ static int avs_es8336_codec_init(struct snd_soc_pcm_runtime *runtime)
 	data = snd_soc_card_get_drvdata(card);
 	num_pins = ARRAY_SIZE(card_headset_pins);
 
-	pins = devm_kmemdup(card->dev, card_headset_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_headset_pins, num_pins,
+				  sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
diff --git a/sound/soc/intel/avs/boards/nau8825.c b/sound/soc/intel/avs/boards/nau8825.c
index 6ea9058fdb2a..0945a539b364 100644
--- a/sound/soc/intel/avs/boards/nau8825.c
+++ b/sound/soc/intel/avs/boards/nau8825.c
@@ -88,7 +88,8 @@ static int avs_nau8825_codec_init(struct snd_soc_pcm_runtime *runtime)
 	jack = snd_soc_card_get_drvdata(card);
 	num_pins = ARRAY_SIZE(card_headset_pins);
 
-	pins = devm_kmemdup(card->dev, card_headset_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_headset_pins, num_pins,
+				  sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
diff --git a/sound/soc/intel/avs/boards/rt274.c b/sound/soc/intel/avs/boards/rt274.c
index 9fcce86c6eb4..bdf36c7c744a 100644
--- a/sound/soc/intel/avs/boards/rt274.c
+++ b/sound/soc/intel/avs/boards/rt274.c
@@ -98,7 +98,8 @@ static int avs_rt274_codec_init(struct snd_soc_pcm_runtime *runtime)
 	jack = snd_soc_card_get_drvdata(card);
 	num_pins = ARRAY_SIZE(card_headset_pins);
 
-	pins = devm_kmemdup(card->dev, card_headset_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_headset_pins, num_pins,
+				  sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
diff --git a/sound/soc/intel/avs/boards/rt286.c b/sound/soc/intel/avs/boards/rt286.c
index f157f2d19efb..f94382389430 100644
--- a/sound/soc/intel/avs/boards/rt286.c
+++ b/sound/soc/intel/avs/boards/rt286.c
@@ -59,7 +59,8 @@ static int avs_rt286_codec_init(struct snd_soc_pcm_runtime *runtime)
 	jack = snd_soc_card_get_drvdata(card);
 	num_pins = ARRAY_SIZE(card_headset_pins);
 
-	pins = devm_kmemdup(card->dev, card_headset_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_headset_pins, num_pins,
+				  sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
diff --git a/sound/soc/intel/avs/boards/rt298.c b/sound/soc/intel/avs/boards/rt298.c
index 1e85242c8dd2..985bfa977edb 100644
--- a/sound/soc/intel/avs/boards/rt298.c
+++ b/sound/soc/intel/avs/boards/rt298.c
@@ -70,7 +70,8 @@ static int avs_rt298_codec_init(struct snd_soc_pcm_runtime *runtime)
 	jack = snd_soc_card_get_drvdata(card);
 	num_pins = ARRAY_SIZE(card_headset_pins);
 
-	pins = devm_kmemdup(card->dev, card_headset_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_headset_pins, num_pins,
+				  sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
diff --git a/sound/soc/intel/avs/boards/rt5663.c b/sound/soc/intel/avs/boards/rt5663.c
index 44f857e90969..fd8b0c915efa 100644
--- a/sound/soc/intel/avs/boards/rt5663.c
+++ b/sound/soc/intel/avs/boards/rt5663.c
@@ -65,7 +65,8 @@ static int avs_rt5663_codec_init(struct snd_soc_pcm_runtime *runtime)
 	jack = &priv->jack;
 	num_pins = ARRAY_SIZE(card_headset_pins);
 
-	pins = devm_kmemdup(card->dev, card_headset_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_headset_pins, num_pins,
+				  sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
diff --git a/sound/soc/intel/avs/boards/rt5682.c b/sound/soc/intel/avs/boards/rt5682.c
index 0dcc6392a0cc..6d7022707ca7 100644
--- a/sound/soc/intel/avs/boards/rt5682.c
+++ b/sound/soc/intel/avs/boards/rt5682.c
@@ -102,7 +102,7 @@ static int avs_rt5682_codec_init(struct snd_soc_pcm_runtime *runtime)
 	jack = snd_soc_card_get_drvdata(card);
 	num_pins = ARRAY_SIZE(card_jack_pins);
 
-	pins = devm_kmemdup(card->dev, card_jack_pins, sizeof(*pins) * num_pins, GFP_KERNEL);
+	pins = devm_kmemdup_array(card->dev, card_jack_pins, num_pins, sizeof(*pins), GFP_KERNEL);
 	if (!pins)
 		return -ENOMEM;
 
-- 
2.35.3

Re: [PATCH v1 1/5] devres: Introduce devm_kmemdup_array()

[Dmitry Torokhov]

Hi Raag,

On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote:
> Introduce '_array' variant of devm_kmemdup() for the users which lack
> multiplication overflow check.

I am not sure that this new helper is needed. Unlike allocators for
brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy
of already existing object, which is supposed to be a valid object and
therefore will have a reasonable size. So there should be no chance for
hitting this overflow unless the caller is completely confused and calls
devm_kmemdup() with random arguments (in which case all bets are off).

> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  include/linux/device.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b4bde8d22697..c31f48d0dde0 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -358,6 +358,16 @@ char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
>  const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);
>  void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
>  	__realloc_size(3);
> +static inline void *devm_kmemdup_array(struct device *dev, const void *src,
> +				       size_t n, size_t size, gfp_t flags)
> +{
> +	size_t bytes;
> +
> +	if (unlikely(check_mul_overflow(n, size, &bytes)))
> +		return NULL;
> +
> +	return devm_kmemdup(dev, src, bytes, flags);
> +}
>  
>  unsigned long devm_get_free_pages(struct device *dev,
>  				  gfp_t gfp_mask, unsigned int order);
> -- 
> 2.35.3
> 

Thanks.

-- 
Dmitry

Re: [PATCH v1 1/5] devres: Introduce devm_kmemdup_array()

[Andy Shevchenko]

On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote:
> On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote:
> > Introduce '_array' variant of devm_kmemdup() for the users which lack
> > multiplication overflow check.
> 
> I am not sure that this new helper is needed. Unlike allocators for
> brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy
> of already existing object, which is supposed to be a valid object and
> therefore will have a reasonable size. So there should be no chance for
> hitting this overflow unless the caller is completely confused and calls
> devm_kmemdup() with random arguments (in which case all bets are off).

Don't we want to have a code more robust even if all what you say applies?
Also this makes the call consistent with zillions of others from the alloc
family of calls in the Linux kernel.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] devres: Introduce devm_kmemdup_array()

[Raag Jadav]

On Mon, Nov 25, 2024 at 09:49:22AM +0200, Andy Shevchenko wrote:
> On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote:
> > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote:
> > > Introduce '_array' variant of devm_kmemdup() for the users which lack
> > > multiplication overflow check.
> > 
> > I am not sure that this new helper is needed. Unlike allocators for
> > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy
> > of already existing object, which is supposed to be a valid object and
> > therefore will have a reasonable size. So there should be no chance for
> > hitting this overflow unless the caller is completely confused and calls
> > devm_kmemdup() with random arguments (in which case all bets are off).
> 
> Don't we want to have a code more robust even if all what you say applies?
> Also this makes the call consistent with zillions of others from the alloc
> family of calls in the Linux kernel.

Agree. Although shooting in the foot is never the expectation, it is
atleast better than having to debug such unexpected cases.

Raag

Re: [PATCH v1 1/5] devres: Introduce devm_kmemdup_array()

[Dmitry Torokhov]

On Mon, Nov 25, 2024 at 05:08:13PM +0200, Raag Jadav wrote:
> On Mon, Nov 25, 2024 at 09:49:22AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote:
> > > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote:
> > > > Introduce '_array' variant of devm_kmemdup() for the users which lack
> > > > multiplication overflow check.
> > > 
> > > I am not sure that this new helper is needed. Unlike allocators for
> > > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy
> > > of already existing object, which is supposed to be a valid object and
> > > therefore will have a reasonable size. So there should be no chance for
> > > hitting this overflow unless the caller is completely confused and calls
> > > devm_kmemdup() with random arguments (in which case all bets are off).
> > 
> > Don't we want to have a code more robust even if all what you say applies?
> > Also this makes the call consistent with zillions of others from the alloc
> > family of calls in the Linux kernel.

Having a clean API is fine, just do not bill it as something that is
"safer". As I mentioned, unlike other allocators this one is supposed to
operate with a valid source object and size passed to devm_kmemdup()
should not exceed the size of the source object. There is no chance of
overflowing.

> 
> Agree. Although shooting in the foot is never the expectation, it is
> atleast better than having to debug such unexpected cases.


Then maybe have a BUG() there instead of returning NULL? I know BUG()s
are frowned upon, but I think in this case overflow is really an
indicator of a hard error by the caller which is passing garbage
arguments to this function.

Hm, I see we have kmemdup_array() already. Ok. How about making your
devm_kmemdup_array() be similar to kmemdup_array()?

static inline void *devm_kmemdup_array(struct device *dev, const void *src,
				       size_t n, size_t size, gfp_t flags)
{
	return devm_kmemdup(dev, src, size_mul(size, n), flags);
}

This will trigger a warning on a too large order of allocation in
mm/page_alloc.c::__alloc_pages_noprof().

Thanks.

-- 
Dmitry

Re: [PATCH v1 1/5] devres: Introduce devm_kmemdup_array()

[Andy Shevchenko]

On Mon, Nov 25, 2024 at 08:29:10AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 25, 2024 at 05:08:13PM +0200, Raag Jadav wrote:
> > On Mon, Nov 25, 2024 at 09:49:22AM +0200, Andy Shevchenko wrote:
> > > On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote:
> > > > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote:

...

> > > > > Introduce '_array' variant of devm_kmemdup() for the users which lack
> > > > > multiplication overflow check.
> > > > 
> > > > I am not sure that this new helper is needed. Unlike allocators for
> > > > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy
> > > > of already existing object, which is supposed to be a valid object and
> > > > therefore will have a reasonable size. So there should be no chance for
> > > > hitting this overflow unless the caller is completely confused and calls
> > > > devm_kmemdup() with random arguments (in which case all bets are off).
> > > 
> > > Don't we want to have a code more robust even if all what you say applies?
> > > Also this makes the call consistent with zillions of others from the alloc
> > > family of calls in the Linux kernel.
> 
> Having a clean API is fine, just do not bill it as something that is
> "safer". As I mentioned, unlike other allocators this one is supposed to
> operate with a valid source object and size passed to devm_kmemdup()
> should not exceed the size of the source object. There is no chance of
> overflowing.

Agree.

> > Agree. Although shooting in the foot is never the expectation, it is
> > atleast better than having to debug such unexpected cases.
> 
> Then maybe have a BUG() there instead of returning NULL? I know BUG()s
> are frowned upon, but I think in this case overflow is really an
> indicator of a hard error by the caller which is passing garbage
> arguments to this function.
> 
> Hm, I see we have kmemdup_array() already. Ok. How about making your
> devm_kmemdup_array() be similar to kmemdup_array()?
> 
> static inline void *devm_kmemdup_array(struct device *dev, const void *src,
> 				       size_t n, size_t size, gfp_t flags)
> {
> 	return devm_kmemdup(dev, src, size_mul(size, n), flags);
> }
> 
> This will trigger a warning on a too large order of allocation in
> mm/page_alloc.c::__alloc_pages_noprof().

This is nice! I have overlooked that kmemdup_array() uses size_mul()
instead of a check. Raag, can you rebuild your series on this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] ASoC: Intel: avs: use devm_kmemdup_array()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 163 bytes --]

On Sun, Nov 24, 2024 at 01:35:27AM +0530, Raag Jadav wrote:
> Convert to use devm_kmemdup_array() which is more robust.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/5] devres: Introduce devm_kmemdup_array()

[Raag Jadav]

On Mon, Nov 25, 2024 at 07:13:06PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 08:29:10AM -0800, Dmitry Torokhov wrote:
> > On Mon, Nov 25, 2024 at 05:08:13PM +0200, Raag Jadav wrote:
> > > On Mon, Nov 25, 2024 at 09:49:22AM +0200, Andy Shevchenko wrote:
> > > > On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote:
> > > > > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote:
> 
> ...
> 
> > > > > > Introduce '_array' variant of devm_kmemdup() for the users which lack
> > > > > > multiplication overflow check.
> > > > > 
> > > > > I am not sure that this new helper is needed. Unlike allocators for
> > > > > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy
> > > > > of already existing object, which is supposed to be a valid object and
> > > > > therefore will have a reasonable size. So there should be no chance for
> > > > > hitting this overflow unless the caller is completely confused and calls
> > > > > devm_kmemdup() with random arguments (in which case all bets are off).
> > > > 
> > > > Don't we want to have a code more robust even if all what you say applies?
> > > > Also this makes the call consistent with zillions of others from the alloc
> > > > family of calls in the Linux kernel.
> > 
> > Having a clean API is fine, just do not bill it as something that is
> > "safer". As I mentioned, unlike other allocators this one is supposed to
> > operate with a valid source object and size passed to devm_kmemdup()
> > should not exceed the size of the source object. There is no chance of
> > overflowing.
> 
> Agree.
> 
> > > Agree. Although shooting in the foot is never the expectation, it is
> > > atleast better than having to debug such unexpected cases.
> > 
> > Then maybe have a BUG() there instead of returning NULL? I know BUG()s
> > are frowned upon, but I think in this case overflow is really an
> > indicator of a hard error by the caller which is passing garbage
> > arguments to this function.
> > 
> > Hm, I see we have kmemdup_array() already. Ok. How about making your
> > devm_kmemdup_array() be similar to kmemdup_array()?
> > 
> > static inline void *devm_kmemdup_array(struct device *dev, const void *src,
> > 				       size_t n, size_t size, gfp_t flags)
> > {
> > 	return devm_kmemdup(dev, src, size_mul(size, n), flags);
> > }
> > 
> > This will trigger a warning on a too large order of allocation in
> > mm/page_alloc.c::__alloc_pages_noprof().
> 
> This is nice! I have overlooked that kmemdup_array() uses size_mul()
> instead of a check. Raag, can you rebuild your series on this?

Sure.

Raag