| Message ID | b945de99-815a-b380-e13c-17b01e0febad@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Stephen Boyd |
| Headers | show |
Hi Markus, On Wed, Sep 14, 2016 at 10:03 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 14 Sep 2016 21:30:27 +0200 > > The kfree() function was called in up to two cases > by the cpg_mstp_clocks_init() function during error handling even if > the passed variable contained a null pointer. It's perfectly legal to call kfree() on a NULL pointer. > * Split a condition check for memory allocation failures so that > each pointer from these function calls will be checked immediately. > > See also background information: > Topic "CWE-754: Improper check for unusual or exceptional conditions" > Link: https://cwe.mitre.org/data/definitions/754.html > > * Return directly after a call of the function "kzalloc" failed > at the beginning. Both calls are already close together. > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/clk/renesas/clk-mstp.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) In addition, your patch increases the LoC, IMHO without improving the code. > > diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c > index 1fdc44b..6c82e0e 100644 > --- a/drivers/clk/renesas/clk-mstp.c > +++ b/drivers/clk/renesas/clk-mstp.c > @@ -167,10 +167,12 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) > unsigned int i; > > group = kzalloc(sizeof(*group), GFP_KERNEL); > + if (!group) > + return; > + > clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL); > - if (group == NULL || clks == NULL) { > + if (!clks) { > kfree(group); > - kfree(clks); > return; > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> It's perfectly legal to call kfree() on a NULL pointer. I know this function property well. >> * Split a condition check for memory allocation failures so that >> each pointer from these function calls will be checked immediately. >> >> See also background information: >> Topic "CWE-754: Improper check for unusual or exceptional conditions" >> Link: https://cwe.mitre.org/data/definitions/754.html >> >> * Return directly after a call of the function "kzalloc" failed >> at the beginning. > > Both calls are already close together. Can it be that an other software development concern is eventually overlooked because of this "neighbourship" (or is categorised with a lower priority)? I suggest to reconsider this design detail if it is really acceptable for the safe implementation of such a software module. * How much will it matter in general that one function call was performed in this use case without checking its return values immediately? * Should it usually be determined quicker if a required resource like memory could be acquired before trying the next allocation? > In addition, your patch increases the LoC, IMHO without improving the code. I find this consequence still debatable. >> diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c >> index 1fdc44b..6c82e0e 100644 >> --- a/drivers/clk/renesas/clk-mstp.c >> +++ b/drivers/clk/renesas/clk-mstp.c >> @@ -167,10 +167,12 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) >> unsigned int i; >> >> group = kzalloc(sizeof(*group), GFP_KERNEL); >> + if (!group) >> + return; >> + >> clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL); >> - if (group == NULL || clks == NULL) { >> + if (!clks) { >> kfree(group); >> - kfree(clks); >> return; >> } Is this update suggestion worth for another look? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 15, 2016 at 10:40 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> * Split a condition check for memory allocation failures so that >>> each pointer from these function calls will be checked immediately. >>> >>> See also background information: >>> Topic "CWE-754: Improper check for unusual or exceptional conditions" >>> Link: https://cwe.mitre.org/data/definitions/754.html >>> >>> * Return directly after a call of the function "kzalloc" failed >>> at the beginning. >> >> Both calls are already close together. > > Can it be that an other software development concern is eventually > overlooked because of this "neighbourship" (or is categorised with > a lower priority)? > > I suggest to reconsider this design detail if it is really acceptable > for the safe implementation of such a software module. > > * How much will it matter in general that one function call was performed > in this use case without checking its return values immediately? > > * Should it usually be determined quicker if a required resource like > memory could be acquired before trying the next allocation? Note that if memory allocation fails in this driver, the system won't boot at all. So even not checking for allocation failures at all could be acceptable. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> * Should it usually be determined quicker if a required resource like >> memory could be acquired before trying the next allocation? > > Note that if memory allocation fails in this driver, the system won't > boot at all. Thanks for this information. > So even not checking for allocation failures at all could be acceptable. I find this opinion interesting somehow. I would generally prefer to check return values from various function calls immediately instead of keeping the discussed source code structure unchanged. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c index 1fdc44b..6c82e0e 100644 --- a/drivers/clk/renesas/clk-mstp.c +++ b/drivers/clk/renesas/clk-mstp.c @@ -167,10 +167,12 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) unsigned int i; group = kzalloc(sizeof(*group), GFP_KERNEL); + if (!group) + return; + clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL); - if (group == NULL || clks == NULL) { + if (!clks) { kfree(group); - kfree(clks); return; }