| Message ID | 1542803310-32673-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] clk: zynq: do not allow kmalloc failure | expand |
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.
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
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.
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
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;