| Message ID | 1543658470-20887-1-git-send-email-hofrat@osadl.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: mux: demux-pinctrl: handle failure case of devm_kstrdup() | expand |
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); >
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); > > >
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); > > >
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); >>> >>
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); >
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
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
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); > > >
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); >>> >>
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);
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(+)