Re: [PATCH] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

[Andrew Lunn]

On Mon, Sep 09, 2024 at 05:17:08PM +0300, Alexandra Diupina wrote:
> Hello, Andrew!
> 
> 
> 09/09/24 17:02, Andrew Lunn пишет:
> > On Mon, Sep 09, 2024 at 04:38:07PM +0300, Alexandra Diupina wrote:
> > > get_div() may return zero, so it is necessary to check
> > > before calling DIV_ROUND_UP_ULL().
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > > 
> > > Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
> > > Signed-off-by: Alexandra Diupina <adiupina@xxxxxxxxxxxxx>
> > > ---
> > >   drivers/clk/mvebu/armada-37xx-periph.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> > > index 8701a58a5804..d0e1d591e4f2 100644
> > > --- a/drivers/clk/mvebu/armada-37xx-periph.c
> > > +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> > > @@ -343,7 +343,10 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
> > >   	div = get_div(double_div->reg1, double_div->shift1);
> > >   	div *= get_div(double_div->reg2, double_div->shift2);
> > > -	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> > > +	if (!div)
> > > +		return 0;
> > Looking at this code, it seems to me some fundamental assumption has
> > gone wrong here, if the dividers are 0. We want to know about this,
> > and a kernel taking a / 0 exception would be a good way to indicate
> > something is very wrong. Won't returning 0 just hide the problem, not
> > make it obvious?
> > 
> > Checking for a /0 on user input makes a lot of sense, but here, i
> > think you are just hiding bugs. Please consider this when making
> > similar changes in other parts of the kernel. Why has a /0 happened?
> > 
> > Tools like SVACE just point at possible problems. You then need to
> > look at them in detail, understand the context, and decide on the
> > proper fix, which might actually be, a /0 is good.
> > 
> > 	Andrew
> 
> The value of div depends on double_div->reg1, double_div->reg2,
> double_div->shift1, double_div->shift2.
> The fields reg1, reg2, shift1, shift2 in the clk_double_div structure
> are filled using the PERIPH_DOUBLEDIV macro, which is called from the
> PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).
> 
> It is not clear what values can be contained in the registers at the
> addresses DIV_SEL0, DIV_SEL1, DIV_SEL2 (can readl() and some bit
> operations give a value > 6 in get_div()), so the final value of div can be
> zero.
> 
> I used just return 0, since the recalc_rate field in the clk_ops structure
> has a comment "If the driver cannot figure out a rate for this clock,
> it must return 0.".

This is the sort of explanation what should be placed into the commit
message. It explains the 'Why?' of the change you made, which you
cannot determine from looking at the change itself.

> I'll fix it to kernel exception, thanks for the tip.

So giving your explanation, i can see why you went for return 0. But i
guess that comment is actually about not being able to ask the
hardware what it is doing. In this case, we can ask it, but we are
getting non-sensible values from it. So i think we should be reporting
this somehow.

	Andrew