Re: [PATCH] i2c: mux: demux-pinctrl: handle failure case of devm_kstrdup()

[Peter Rosin <peda@axentia.se>]

Ho hmm. Had another look at this patch, or rather, the context of the
patch so not really related, but...

On 2018-12-01 11:01, Nicholas Mc Guire wrote:
> devm_kstrdup() may return NULL if internal allocation failed.
> Thus using  name, value  is unsafe without being checked. As
> i2c_demux_pinctrl_probe() can return -ENOMEM in other cases
> a dev_err() message is included to make the failure location
> clear.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: e35478eac030 ("i2c: mux: demux-pinctrl: run properly with multiple instances")
> ---
> 
> Problem located with experimental coccinelle script
> 
> Q: The use of devm_kstrdup() seems a bit odd while technically not wrong,
>    personally I think devm_kasprintf() would be more suitable here.
> 
> Patch was compile tested with: multi_v7_defconfig 
> (implies I2C_DEMUX_PINCTRL=y)
> 
> Patch is against 4.20-rc4 (localversion-next is next-20181130)
> 
>  drivers/i2c/muxes/i2c-demux-pinctrl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 035032e..c466999 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -244,6 +244,12 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
>  
>  		props[i].name = devm_kstrdup(&pdev->dev, "status", GFP_KERNEL);
>  		props[i].value = devm_kstrdup(&pdev->dev, "ok", GFP_KERNEL);

It seemed very dubious to use devm_kstrdup here, since

1. if the consumer is not freeing the strings it would be sufficient with just
  		props[i].name = "status";
  		props[i].value = "ok";

2. if the consumer is freeing the strings, it is very bad to free them twice
   which is what happens with the devm_ prefix.

So, there is no case, AFAICT, where it is sane to use devm_kstrdup.

Therefore I had a look at the code, and to me it seems as if the consumer
very much frees the strings, which means that we are in case 2, and that
the above should be ordinary kstrdup calls.

Am I missing something?

Cheers,
Peter

> +		if (!props[i].name || !props[i].value) {
> +			dev_err(&pdev->dev,
> +				"chan %d name, value allocation failed\n", i);
> +			err = -ENOMEM;
> +			goto err_rollback;
> +		}
>  		props[i].length = 3;
>  
>  		of_changeset_init(&priv->chan[i].chgset);
>