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

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

[Nicholas Mc Guire]

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);
+		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);
-- 
2.1.4

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

[Peter Rosin]

Hi!

This patch looks like a good idea. However, a nitpick below.

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);
> +		if (!props[i].name || !props[i].value) {
> +			dev_err(&pdev->dev,
> +				"chan %d name, value allocation failed\n", i);

Please drop this memory allocation failure message. You should get such a
message from devm_kstrdup.

Cheers,
Peter

> +			err = -ENOMEM;
> +			goto err_rollback;
> +		}
>  		props[i].length = 3;
>  
>  		of_changeset_init(&priv->chan[i].chgset);
> 

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

[Nicholas Mc Guire]

On Tue, Dec 04, 2018 at 11:16:59AM +0000, Peter Rosin wrote:
> Hi!
> 
> This patch looks like a good idea. However, a nitpick below.
> 
> 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);
> > +		if (!props[i].name || !props[i].value) {
> > +			dev_err(&pdev->dev,
> > +				"chan %d name, value allocation failed\n", i);
> 
> Please drop this memory allocation failure message. You should get such a
> message from devm_kstrdup.
>
thanks for the review - will drop the dev_err() then an resend 

thx!
hofrat
 
> 
> > +			err = -ENOMEM;
> > +			goto err_rollback;
> > +		}
> >  		props[i].length = 3;
> >  
> >  		of_changeset_init(&priv->chan[i].chgset);
> > 
> 

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

[Nicholas Mc Guire]

On Tue, Dec 04, 2018 at 11:16:59AM +0000, Peter Rosin wrote:
> Hi!
> 
> This patch looks like a good idea. However, a nitpick below.
> 
> 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);
> > +		if (!props[i].name || !props[i].value) {
> > +			dev_err(&pdev->dev,
> > +				"chan %d name, value allocation failed\n", i);
> 
> Please drop this memory allocation failure message. You should get such a
> message from devm_kstrdup.
>

hm...tried to figure out where that message would be comming
from - but I could not find any point in the call tree that
would issue such a message ?

 devm_kstrdup() 
   -> devm_kmalloc()
        -> alloc_dr()
             --> kmalloc_track_caller() (non-NUMA)
             |     -> __kmalloc_node()
             |        -> __do_kmalloc_node()
             `-> __kmalloc_node_track_caller() (NUMA)
                   -> __do_kmalloc_node()

 __do_kmalloc_node() seems like it simply returns NULL but
 issues no failure message.
 Am I overlooking something ? 

thx!
hofrat 
 
> Cheers,
> Peter
> 
> > +			err = -ENOMEM;
> > +			goto err_rollback;
> > +		}
> >  		props[i].length = 3;
> >  
> >  		of_changeset_init(&priv->chan[i].chgset);
> > 
> 

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

[Peter Rosin]

On 2018-12-04 13:13, Nicholas Mc Guire wrote:
> On Tue, Dec 04, 2018 at 11:16:59AM +0000, Peter Rosin wrote:
>> Hi!
>>
>> This patch looks like a good idea. However, a nitpick below.
>>
>> 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);
>>> +		if (!props[i].name || !props[i].value) {
>>> +			dev_err(&pdev->dev,
>>> +				"chan %d name, value allocation failed\n", i);
>>
>> Please drop this memory allocation failure message. You should get such a
>> message from devm_kstrdup.
>>
> 
> hm...tried to figure out where that message would be comming
> from - but I could not find any point in the call tree that
> would issue such a message ?
> 
>  devm_kstrdup() 
>    -> devm_kmalloc()
>         -> alloc_dr()
>              --> kmalloc_track_caller() (non-NUMA)
>              |     -> __kmalloc_node()
>              |        -> __do_kmalloc_node()
>              `-> __kmalloc_node_track_caller() (NUMA)
>                    -> __do_kmalloc_node()
> 
>  __do_kmalloc_node() seems like it simply returns NULL but
>  issues no failure message.
>  Am I overlooking something ? 

Well, I don't know the details, but checkpatch will warn about simple
error messages on devm_kstrdup failure (if I read the checkpatch source
correctly). But in this case there are two parallel conditions in the
if and hence checkpatch stumbles, but gist is the same, you should not
sprinkle messages on memory allocation failure.

Cheers,
Peter

> thx!
> hofrat 
>  
>> Cheers,
>> Peter
>>
>>> +			err = -ENOMEM;
>>> +			goto err_rollback;
>>> +		}
>>>  		props[i].length = 3;
>>>  
>>>  		of_changeset_init(&priv->chan[i].chgset);
>>>
>>

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

[Nicholas Mc Guire]

On Tue, Dec 04, 2018 at 01:49:11PM +0000, Peter Rosin wrote:
> On 2018-12-04 13:13, Nicholas Mc Guire wrote:
> > On Tue, Dec 04, 2018 at 11:16:59AM +0000, Peter Rosin wrote:
> >> Hi!
> >>
> >> This patch looks like a good idea. However, a nitpick below.
> >>
> >> 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);
> >>> +		if (!props[i].name || !props[i].value) {
> >>> +			dev_err(&pdev->dev,
> >>> +				"chan %d name, value allocation failed\n", i);
> >>
> >> Please drop this memory allocation failure message. You should get such a
> >> message from devm_kstrdup.
> >>
> > 
> > hm...tried to figure out where that message would be comming
> > from - but I could not find any point in the call tree that
> > would issue such a message ?
> > 
> >  devm_kstrdup() 
> >    -> devm_kmalloc()
> >         -> alloc_dr()
> >              --> kmalloc_track_caller() (non-NUMA)
> >              |     -> __kmalloc_node()
> >              |        -> __do_kmalloc_node()
> >              `-> __kmalloc_node_track_caller() (NUMA)
> >                    -> __do_kmalloc_node()
> > 
> >  __do_kmalloc_node() seems like it simply returns NULL but
> >  issues no failure message.
> >  Am I overlooking something ? 
> 
> Well, I don't know the details, but checkpatch will warn about simple
> error messages on devm_kstrdup failure (if I read the checkpatch source
> correctly). But in this case there are two parallel conditions in the
> if and hence checkpatch stumbles, but gist is the same, you should not
> sprinkle messages on memory allocation failure.
>
not in this case - atleast checkpatch --strict on the original patch
did not issue any complaint to that ends. But yes - you
are right that the intent in checkpatch is clear and this should not
be carrying a failure message.

thx!
hofrat 

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

[Peter Rosin]

On 2018-12-04 15:25, Nicholas Mc Guire wrote:
> On Tue, Dec 04, 2018 at 01:49:11PM +0000, Peter Rosin wrote:
>> On 2018-12-04 13:13, Nicholas Mc Guire wrote:
>>> On Tue, Dec 04, 2018 at 11:16:59AM +0000, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> This patch looks like a good idea. However, a nitpick below.
>>>>
>>>> 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);
>>>>> +		if (!props[i].name || !props[i].value) {
>>>>> +			dev_err(&pdev->dev,
>>>>> +				"chan %d name, value allocation failed\n", i);
>>>>
>>>> Please drop this memory allocation failure message. You should get such a
>>>> message from devm_kstrdup.
>>>>
>>>
>>> hm...tried to figure out where that message would be comming
>>> from - but I could not find any point in the call tree that
>>> would issue such a message ?
>>>
>>>  devm_kstrdup() 
>>>    -> devm_kmalloc()
>>>         -> alloc_dr()
>>>              --> kmalloc_track_caller() (non-NUMA)
>>>              |     -> __kmalloc_node()
>>>              |        -> __do_kmalloc_node()
>>>              `-> __kmalloc_node_track_caller() (NUMA)
>>>                    -> __do_kmalloc_node()
>>>
>>>  __do_kmalloc_node() seems like it simply returns NULL but
>>>  issues no failure message.
>>>  Am I overlooking something ? 
>>
>> Well, I don't know the details, but checkpatch will warn about simple
>> error messages on devm_kstrdup failure (if I read the checkpatch source
>> correctly). But in this case there are two parallel conditions in the
>> if and hence checkpatch stumbles, but gist is the same, you should not
>> sprinkle messages on memory allocation failure.
>>
> not in this case - atleast checkpatch --strict on the original patch
> did not issue any complaint to that ends. But yes - you
> are right that the intent in checkpatch is clear and this should not
> be carrying a failure message.

Yes, this is exactly what I said, checkpatch stumbles since there are
two conflated tests in one if statement and checkpatch is not smart
so does not pick up on that.

Cheers,
Peter

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

[Peter Rosin]

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);
> 

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

[Nicholas Mc Guire]

On Tue, Dec 04, 2018 at 02:11:33PM +0000, Peter Rosin wrote:
> 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
>

yup - which is where the question in the initial pach came
from I felt that this should better be a devm_kasprint() - I
did not understand why it was kstrdup here in the first
place - technically it is not wrong though as the source is not RO
so it will allocate and copy the original string and thus
effectively it behaves like devm_kasprintf
 
> 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.

Why would it be freed twice ? the pointer returned is a seperately allocated objects ?
that needs to be indepdently freed (this is not the devm_kstrdup_const() case)

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

I think the only technical difference between devm_kstrdup and devm_kasprintf
is effectively a memcpy vs vsnprintf - so here I think devm_kasprintf would
also be the more suitable call to use.

thx!
hofrat

> 
> 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);
> > 
> 

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

[Peter Rosin]

On 2018-12-04 15:36, Nicholas Mc Guire wrote:
> On Tue, Dec 04, 2018 at 02:11:33PM +0000, Peter Rosin wrote:
>> 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
>>
> 
> yup - which is where the question in the initial pach came
> from I felt that this should better be a devm_kasprint() - I
> did not understand why it was kstrdup here in the first
> place - technically it is not wrong though as the source is not RO
> so it will allocate and copy the original string and thus
> effectively it behaves like devm_kasprintf

devm_kasprintf would be as bad as devm_kstrdup here (if I read the code
right). Both allocates memory that is freed both by the devm_... machinery
and by the consumer. Maybe I should have said "props consumer" instead of
just "consumer" to make it clear that I'm talking about the dynamic of
code as consumer.

>  
>> 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.
> 
> Why would it be freed twice ? the pointer returned is a seperately allocated objects ?
> that needs to be indepdently freed (this is not the devm_kstrdup_const() case)

The props[i] struct is sent off to the "consumer" with the call to
of_changeset_update_property. After that (if I read it right) the
changeset owns the prop and will free it, and its content, as it is
cleaned up. It is therefore wrong for this driver to also clean up
that memory (by using devm_ prefixed functions for these two
entities).

>>
>> So, there is no case, AFAICT, where it is sane to use devm_kstrdup.
> 
> I think the only technical difference between devm_kstrdup and devm_kasprintf
> is effectively a memcpy vs vsnprintf - so here I think devm_kasprintf would
> also be the more suitable call to use.

That difference is not very important, but strdup should always be faster
than asprintf, so I see no point in not using strdup.

> thx!
> hofrat
> 
>>
>> 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);
>>>
>>