From: Peter Rosin <peda@axentia.se>
To: Nicholas Mc Guire <der.herr@hofr.at>
Cc: Nicholas Mc Guire <hofrat@osadl.org>,
Wolfram Sang <wsa+renesas@sang-engineering.com>,
"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] i2c: mux: demux-pinctrl: handle failure case of devm_kstrdup()
Date: Tue, 4 Dec 2018 14:29:39 +0000 [thread overview]
Message-ID: <ddd13632-2d1f-f87c-f0cc-1bd3d1d09452@axentia.se> (raw)
In-Reply-To: <20181204142504.GA16001@osadl.at>
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
next prev parent reply other threads:[~2018-12-04 14:29 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-01 10:01 [PATCH] i2c: mux: demux-pinctrl: handle failure case of devm_kstrdup() Nicholas Mc Guire
2018-12-04 11:16 ` Peter Rosin
2018-12-04 11:43 ` Nicholas Mc Guire
2018-12-04 12:13 ` Nicholas Mc Guire
2018-12-04 13:49 ` Peter Rosin
2018-12-04 14:25 ` Nicholas Mc Guire
2018-12-04 14:29 ` Peter Rosin [this message]
2018-12-04 14:11 ` Peter Rosin
2018-12-04 14:36 ` Nicholas Mc Guire
2018-12-04 14:50 ` Peter Rosin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ddd13632-2d1f-f87c-f0cc-1bd3d1d09452@axentia.se \
--to=peda@axentia.se \
--cc=der.herr@hofr.at \
--cc=hofrat@osadl.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=wsa+renesas@sang-engineering.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).