[PATCH V2] clk: zynq: do not allow kmalloc failure

[PATCH V2] clk: zynq: do not allow kmalloc failure

[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

Re: [PATCH V2] clk: zynq: do not allow kmalloc failure

[Stephen Boyd]

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.

Re: [PATCH V2] clk: zynq: do not allow kmalloc failure

[Nicholas Mc Guire]

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

Re: [PATCH V2] clk: zynq: do not allow kmalloc failure

[Stephen Boyd]

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.

Re: [PATCH V2] clk: zynq: do not allow kmalloc failure

[Nicholas Mc Guire]

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