Message ID | 20171218170007.28042-1-jbrunet@baylibre.com |
---|---|
State | Superseded |
Headers | show |
Series | clk: check ops pointer on clock register | expand |
On 12/18, Jerome Brunet wrote: > Nothing really prevents a provider from (trying to) register a clock > without providing the clock ops structure. > > We do check the individual fields before using them, but not the > structure pointer itself. This may have the usual nasty consequences when > the pointer is dereferenced, mostly likely when checking one the field > during the initialization. Yes, that nasty consequence should be a kernel oops, and the developer should notice that before submitting the driver for inclusion. I don't think we really care to return an error here if this happens. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote: > On 12/18, Jerome Brunet wrote: > > Nothing really prevents a provider from (trying to) register a clock > > without providing the clock ops structure. > > > > We do check the individual fields before using them, but not the > > structure pointer itself. This may have the usual nasty consequences when > > the pointer is dereferenced, mostly likely when checking one the field > > during the initialization. > > Yes, that nasty consequence should be a kernel oops, Precisely > and the > developer should notice that before submitting the driver for > inclusion. Agreed. But people may make mistakes, which is why (at least partly) we do checks, isn't it ? > I don't think we really care to return an error here > if this happens. > I don't understand why we would let a oops happen when can catch the error properly ?
Hi Jerome & Stephen, On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote: >> On 12/18, Jerome Brunet wrote: >> > Nothing really prevents a provider from (trying to) register a clock >> > without providing the clock ops structure. >> > >> > We do check the individual fields before using them, but not the >> > structure pointer itself. This may have the usual nasty consequences when >> > the pointer is dereferenced, mostly likely when checking one the field >> > during the initialization. >> >> Yes, that nasty consequence should be a kernel oops, > > Precisely > >> and the >> developer should notice that before submitting the driver for >> inclusion. > > Agreed. But people may make mistakes, which is why (at least partly) we > do checks, isn't it ? Agreed the developers should test before submitting, but procedurally generated clocks (e.g. registering clocks in a loop using a predictable register map, etc) could lead to a situation where a developer doesn't test every possible iteration. Hypothetical, but easy easy easy to fix with Jerome's patch. > >> I don't think we really care to return an error here >> if this happens. >> > > I don't understand why we would let a oops happen when can catch the error > properly ? > Agreed with Jerome on this one. Let's flip it on its head: any downside to this patch? If not I can merge. Regards, Mike
On 12/18, Michael Turquette wrote: > Hi Jerome & Stephen, > > On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote: > >> On 12/18, Jerome Brunet wrote: > >> > Nothing really prevents a provider from (trying to) register a clock > >> > without providing the clock ops structure. > >> > > >> > We do check the individual fields before using them, but not the > >> > structure pointer itself. This may have the usual nasty consequences when > >> > the pointer is dereferenced, mostly likely when checking one the field > >> > during the initialization. > >> > >> Yes, that nasty consequence should be a kernel oops, > > > > Precisely > > > >> and the > >> developer should notice that before submitting the driver for > >> inclusion. > > > > Agreed. But people may make mistakes, which is why (at least partly) we > > do checks, isn't it ? > > Agreed the developers should test before submitting, but procedurally > generated clocks (e.g. registering clocks in a loop using a > predictable register map, etc) could lead to a situation where a > developer doesn't test every possible iteration. > > Hypothetical, but easy easy easy to fix with Jerome's patch. > > > > >> I don't think we really care to return an error here > >> if this happens. > >> > > > > I don't understand why we would let a oops happen when can catch the error > > properly ? > > > > Agreed with Jerome on this one. > > Let's flip it on its head: any downside to this patch? If not I can merge. > If code is not checking return values from clk_register(), then an oops turns into a silently ignored error. Hunting that down is going to take some time vs. an oops when we attempt to call the clk ops that aren't there. The idea is fine, but I would change two things. First I would throw a WARN_ON() around the condition so developers notice quicker that something is wrong, and second I would strip off the 'Fixes' tag because this isn't really fixing anything that we need to backport to stable trees. It just converts an oops into a warning. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Mon, 2017-12-18 at 13:06 -0800, Stephen Boyd wrote: > On 12/18, Michael Turquette wrote: > > Hi Jerome & Stephen, > > > > On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote: > > > > On 12/18, Jerome Brunet wrote: > > > > > Nothing really prevents a provider from (trying to) register a clock > > > > > without providing the clock ops structure. > > > > > > > > > > We do check the individual fields before using them, but not the > > > > > structure pointer itself. This may have the usual nasty consequences when > > > > > the pointer is dereferenced, mostly likely when checking one the field > > > > > during the initialization. > > > > > > > > Yes, that nasty consequence should be a kernel oops, > > > > > > Precisely > > > > > > > and the > > > > developer should notice that before submitting the driver for > > > > inclusion. > > > > > > Agreed. But people may make mistakes, which is why (at least partly) we > > > do checks, isn't it ? > > > > Agreed the developers should test before submitting, but procedurally > > generated clocks (e.g. registering clocks in a loop using a > > predictable register map, etc) could lead to a situation where a > > developer doesn't test every possible iteration. > > > > Hypothetical, but easy easy easy to fix with Jerome's patch. > > > > > > > > > I don't think we really care to return an error here > > > > if this happens. > > > > > > > > > > I don't understand why we would let a oops happen when can catch the error > > > properly ? > > > > > > > Agreed with Jerome on this one. > > > > Let's flip it on its head: any downside to this patch? If not I can merge. > > > > If code is not checking return values from clk_register(), then > an oops turns into a silently ignored error. It would really be asking for trouble, wouldn't it ? > Hunting that down is > going to take some time vs. an oops when we attempt to call the > clk ops that aren't there. > > The idea is fine, but I would change two things. First I would > throw a WARN_ON() around the condition so developers notice > quicker that something is wrong, and second I would strip off the > 'Fixes' tag because this isn't really fixing anything that we > need to backport to stable trees. It just converts an oops into a > warning. > Fine by me.
On Mon, Dec 18, 2017 at 8:03 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 12/18, Jerome Brunet wrote: >> Nothing really prevents a provider from (trying to) register a clock >> without providing the clock ops structure. >> >> We do check the individual fields before using them, but not the >> structure pointer itself. This may have the usual nasty consequences when >> the pointer is dereferenced, mostly likely when checking one the field >> during the initialization. > > Yes, that nasty consequence should be a kernel oops, and the > developer should notice that before submitting the driver for > inclusion. I don't think we really care to return an error here > if this happens. +1 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
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8a1860a36c77..275b45664227 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2683,7 +2683,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) ret = -ENOMEM; goto fail_name; } + + if (!hw->init->ops) { + ret = -EINVAL; + goto fail_ops; + } core->ops = hw->init->ops; + if (dev && pm_runtime_enabled(dev)) core->dev = dev; if (dev && dev->driver) @@ -2745,6 +2751,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) kfree_const(core->parent_names[i]); kfree(core->parent_names); fail_parent_names: +fail_ops: kfree_const(core->name); fail_name: kfree(core);
Nothing really prevents a provider from (trying to) register a clock without providing the clock ops structure. We do check the individual fields before using them, but not the structure pointer itself. This may have the usual nasty consequences when the pointer is dereferenced, mostly likely when checking one the field during the initialization. This is fixed by returning an error on clock register if the ops pointer is NULL Fixes: b2476490ef11 ("clk: introduce the common clock framework") Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- Mike, Stephen, I'm not really sure what the Fixes tag should here. From what I could see, we never checked the ops pointer before using it since the beginning of CCF. Regards Jerome drivers/clk/clk.c | 7 +++++++ 1 file changed, 7 insertions(+) -- 2.14.3