* [PATCH V2] clk: zynq: do not allow kmalloc failure
@ 2018-11-21 12:28 Nicholas Mc Guire
2018-11-29 23:45 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Nicholas Mc Guire @ 2018-11-21 12:28 UTC (permalink / raw)
To: Michael Turquette
Cc: Stephen Boyd, Michal Simek, Rob Herring, linux-clk,
linux-arm-kernel, linux-kernel, Nicholas Mc Guire
The kmalloc here is small (< 16 bytes) and occurs during initialization
during system startup here (can not be built as module) thus if this
kmalloc failed it is an indication of something more serious going on
and it is fine to hang the system here rather than cause some harder
to understand error by dereferencing NULL.
Explicitly checking would not make that much sense here as the only
possible reaction would be would BUG() here anyway.
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: 0ee52b157b8e ("clk: zynq: Add clock controller driver")
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
V2: dropped leading spaces from commit message and moved the
compile/tool info out of git history on request of
Michal Simek <michal.simek@xilinx.com> - thanks !
Problem located with experimental coccinelle script
Patch was compile tested with: multi_v7_defconfig (implies
CONFIG_ARCH_ZYNQ=y)
Patch is against 4.20-rc3 (localversion-next is next-20181121)
drivers/clk/zynq/clkc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/zynq/clkc.c b/drivers/clk/zynq/clkc.c
index d7b53ac..014d4a4 100644
--- a/drivers/clk/zynq/clkc.c
+++ b/drivers/clk/zynq/clkc.c
@@ -440,7 +440,7 @@ static void __init zynq_clk_setup(struct device_node *np)
SLCR_GEM1_CLK_CTRL, 0, 0, &gem1clk_lock);
tmp = strlen("mio_clk_00x");
- clk_name = kmalloc(tmp, GFP_KERNEL);
+ clk_name = kmalloc(tmp, GFP_KERNEL | __GFP_NOFAIL);
for (i = 0; i < NUM_MIO_PINS; i++) {
int idx;
--
2.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V2] clk: zynq: do not allow kmalloc failure
2018-11-21 12:28 [PATCH V2] clk: zynq: do not allow kmalloc failure Nicholas Mc Guire
@ 2018-11-29 23:45 ` Stephen Boyd
2018-11-30 7:54 ` Nicholas Mc Guire
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2018-11-29 23:45 UTC (permalink / raw)
To: Michael Turquette, Nicholas Mc Guire
Cc: Michal Simek, Rob Herring, linux-clk, linux-arm-kernel,
linux-kernel, Nicholas Mc Guire
Quoting Nicholas Mc Guire (2018-11-21 04:28:30)
> The kmalloc here is small (< 16 bytes) and occurs during initialization
> during system startup here (can not be built as module) thus if this
> kmalloc failed it is an indication of something more serious going on
> and it is fine to hang the system here rather than cause some harder
> to understand error by dereferencing NULL.
>
> Explicitly checking would not make that much sense here as the only
> possible reaction would be would BUG() here anyway.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 0ee52b157b8e ("clk: zynq: Add clock controller driver")
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> ---
Nak. We don't have any __GFP_NOFAIL in drivers/clk and I don't see a
reason why we would want it here either. Just handle the failure, or
don't care if this is so critical to system boot.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] clk: zynq: do not allow kmalloc failure
2018-11-29 23:45 ` Stephen Boyd
@ 2018-11-30 7:54 ` Nicholas Mc Guire
2018-11-30 8:09 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Nicholas Mc Guire @ 2018-11-30 7:54 UTC (permalink / raw)
To: Stephen Boyd
Cc: Michael Turquette, Nicholas Mc Guire, Michal Simek, Rob Herring,
linux-clk, linux-arm-kernel, linux-kernel
On Thu, Nov 29, 2018 at 03:45:23PM -0800, Stephen Boyd wrote:
> Quoting Nicholas Mc Guire (2018-11-21 04:28:30)
> > The kmalloc here is small (< 16 bytes) and occurs during initialization
> > during system startup here (can not be built as module) thus if this
> > kmalloc failed it is an indication of something more serious going on
> > and it is fine to hang the system here rather than cause some harder
> > to understand error by dereferencing NULL.
> >
> > Explicitly checking would not make that much sense here as the only
> > possible reaction would be would BUG() here anyway.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > Fixes: 0ee52b157b8e ("clk: zynq: Add clock controller driver")
> > Acked-by: Michal Simek <michal.simek@xilinx.com>
> > ---
>
> Nak. We don't have any __GFP_NOFAIL in drivers/clk and I don't see a
> reason why we would want it here either. Just handle the failure, or
> don't care if this is so critical to system boot.
>
It was not motivated by the criticality but by the low probability
and cluttering the code for this case did not seem good to me.
Effectively handling it here means BUG() - so more or less
the same result that hanging it on __GFP_NOFAIL if allocation
was not possible would cause.
Not clear what the objection to __GFP_NOFAIL here is - my understanding
was that it is intended precisely for cases like this - but
I´ll send a V2 handling it with BUG_ON(!clk_name) if that is prefered.
thx!
hofrat
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] clk: zynq: do not allow kmalloc failure
2018-11-30 7:54 ` Nicholas Mc Guire
@ 2018-11-30 8:09 ` Stephen Boyd
2018-11-30 8:29 ` Nicholas Mc Guire
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2018-11-30 8:09 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Michael Turquette, Nicholas Mc Guire, Michal Simek, Rob Herring,
linux-clk, linux-arm-kernel, linux-kernel
Quoting Nicholas Mc Guire (2018-11-29 23:54:53)
> On Thu, Nov 29, 2018 at 03:45:23PM -0800, Stephen Boyd wrote:
> > Quoting Nicholas Mc Guire (2018-11-21 04:28:30)
> > > The kmalloc here is small (< 16 bytes) and occurs during initialization
> > > during system startup here (can not be built as module) thus if this
> > > kmalloc failed it is an indication of something more serious going on
> > > and it is fine to hang the system here rather than cause some harder
> > > to understand error by dereferencing NULL.
> > >
> > > Explicitly checking would not make that much sense here as the only
> > > possible reaction would be would BUG() here anyway.
> > >
> > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > Fixes: 0ee52b157b8e ("clk: zynq: Add clock controller driver")
> > > Acked-by: Michal Simek <michal.simek@xilinx.com>
> > > ---
> >
> > Nak. We don't have any __GFP_NOFAIL in drivers/clk and I don't see a
> > reason why we would want it here either. Just handle the failure, or
> > don't care if this is so critical to system boot.
> >
> It was not motivated by the criticality but by the low probability
> and cluttering the code for this case did not seem good to me.
> Effectively handling it here means BUG() - so more or less
> the same result that hanging it on __GFP_NOFAIL if allocation
> was not possible would cause.
>
> Not clear what the objection to __GFP_NOFAIL here is - my understanding
> was that it is intended precisely for cases like this - but
> I´ll send a V2 handling it with BUG_ON(!clk_name) if that is prefered.
>
Or just WARN() and return. Maybe something else can get far enough to be
helpful.
I would also appreciate if this sort of problem could be caught earlier
in code review and/or with some automated scripting. Debating BUG_ON()
and allocation failures is not what I look forward to doing so please
try to make this exact sort of patch never make it to the list in the
first place.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] clk: zynq: do not allow kmalloc failure
2018-11-30 8:09 ` Stephen Boyd
@ 2018-11-30 8:29 ` Nicholas Mc Guire
0 siblings, 0 replies; 5+ messages in thread
From: Nicholas Mc Guire @ 2018-11-30 8:29 UTC (permalink / raw)
To: Stephen Boyd
Cc: Michael Turquette, Nicholas Mc Guire, Michal Simek, Rob Herring,
linux-clk, linux-arm-kernel, linux-kernel
On Fri, Nov 30, 2018 at 12:09:30AM -0800, Stephen Boyd wrote:
> Quoting Nicholas Mc Guire (2018-11-29 23:54:53)
> > On Thu, Nov 29, 2018 at 03:45:23PM -0800, Stephen Boyd wrote:
> > > Quoting Nicholas Mc Guire (2018-11-21 04:28:30)
> > > > The kmalloc here is small (< 16 bytes) and occurs during initialization
> > > > during system startup here (can not be built as module) thus if this
> > > > kmalloc failed it is an indication of something more serious going on
> > > > and it is fine to hang the system here rather than cause some harder
> > > > to understand error by dereferencing NULL.
> > > >
> > > > Explicitly checking would not make that much sense here as the only
> > > > possible reaction would be would BUG() here anyway.
> > > >
> > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > > Fixes: 0ee52b157b8e ("clk: zynq: Add clock controller driver")
> > > > Acked-by: Michal Simek <michal.simek@xilinx.com>
> > > > ---
> > >
> > > Nak. We don't have any __GFP_NOFAIL in drivers/clk and I don't see a
> > > reason why we would want it here either. Just handle the failure, or
> > > don't care if this is so critical to system boot.
> > >
> > It was not motivated by the criticality but by the low probability
> > and cluttering the code for this case did not seem good to me.
> > Effectively handling it here means BUG() - so more or less
> > the same result that hanging it on __GFP_NOFAIL if allocation
> > was not possible would cause.
> >
> > Not clear what the objection to __GFP_NOFAIL here is - my understanding
> > was that it is intended precisely for cases like this - but
> > I´ll send a V2 handling it with BUG_ON(!clk_name) if that is prefered.
> >
>
> Or just WARN() and return. Maybe something else can get far enough to be
> helpful.
>
> I would also appreciate if this sort of problem could be caught earlier
> in code review and/or with some automated scripting. Debating BUG_ON()
> and allocation failures is not what I look forward to doing so please
> try to make this exact sort of patch never make it to the list in the
> first place.
>
well it was found by a experimental coccinelle script
I´m trying to write up semi-formal specifications for
APIs so that this can be tested automatically and cought early.
If you put in a WARN() here it would still allow for the
NULL pointer to be passt on potentially corupting memory in the following
snprintf()->vsnprintf() which does not seem to check for !buf
thx!
hofrat
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-30 8:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 12:28 [PATCH V2] clk: zynq: do not allow kmalloc failure Nicholas Mc Guire
2018-11-29 23:45 ` Stephen Boyd
2018-11-30 7:54 ` Nicholas Mc Guire
2018-11-30 8:09 ` Stephen Boyd
2018-11-30 8:29 ` Nicholas Mc Guire
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).