linux-kernel - Re: [PATCH v1 1/1] clk: devres: Simplify devres handling functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9e16ddb1c1a697464ce1f5438ab9ca31.sboyd@kernel.org>
Date: Tue, 27 Aug 2024 17:19:04 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Michael Turquette <mturquette@...libre.com>, Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH v1 1/1] clk: devres: Simplify devres handling functions

Quoting Andy Shevchenko (2024-08-22 08:58:22)
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 90e6078fb6e1..f03d60706a85 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -28,15 +30,13 @@ static struct clk *__devm_clk_get(struct device *dev, const char *id,
>         struct clk *clk;
>         int ret;
>  
> -       state = devres_alloc(devm_clk_release, sizeof(*state), GFP_KERNEL);
> +       state = devm_kmalloc(dev, sizeof(*state), GFP_KERNEL);

When is this allocation freed if the get() fails? When the calling
device driver detaches?

>         if (!state)
>                 return ERR_PTR(-ENOMEM);
>  
>         clk = get(dev, id);
> -       if (IS_ERR(clk)) {
> -               ret = PTR_ERR(clk);
> -               goto err_clk_get;
> -       }
> +       if (IS_ERR(clk))
> +               return clk;
>  
>         if (init) {
>                 ret = init(clk);
> @@ -47,16 +47,14 @@ static struct clk *__devm_clk_get(struct device *dev, const char *id,
>         state->clk = clk;
>         state->exit = exit;
>  
> -       devres_add(dev, state);
> +       ret = devm_add_action_or_reset(dev, devm_clk_release, state);
> +       if (ret)
> +               goto err_clk_init;

Shouldn't we return an error here? Otherwise we call clk_put() twice?

>  
>         return clk;
>  
>  err_clk_init:
> -
>         clk_put(clk);
> -err_clk_get:
> -
> -       devres_free(state);
>         return ERR_PTR(ret);
>  }
>  

I stopped reading, sorry! If you want to do this, please add a bunch of
KUnit tests.