linux-kernel - Re: [PATCH 5/6] regulator: core: propagate error out ouf regulator_lock

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: <CAD=FV=XftP6icTWK-xEPJvBiuXLeCDpy1ORiwedxNtw7z2Rp6w@mail.gmail.com>
Date:   Mon, 21 Aug 2023 09:50:53 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        Dmitry Osipenko <digetx@...il.com>
Subject: Re: [PATCH 5/6] regulator: core: propagate error out ouf regulator_lock_two()

Hi,

On Sat, Aug 19, 2023 at 3:46 PM Michał Mirosław <mirq-linux@...e.qmqm.pl> wrote:
>
> Fix up error paths from regulator_lock_two(): although it should not
> fail, returning with half-locked state after issuing a WARN() asks
> for even more trouble.
>
> Fixes: cba6cfdc7c3f ("regulator: core: Avoid lockdep reports when resolving supplies")
> Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> ---
>  drivers/regulator/core.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index de434d550937..d8c2277cea36 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -197,9 +197,9 @@ static void regulator_unlock(struct regulator_dev *rdev)
>   *
>   * Locks both rdevs using the regulator_ww_class.
>   */
> -static void regulator_lock_two(struct regulator_dev *rdev1,
> -                              struct regulator_dev *rdev2,
> -                              struct ww_acquire_ctx *ww_ctx)
> +static int regulator_lock_two(struct regulator_dev *rdev1,
> +                             struct regulator_dev *rdev2,
> +                             struct ww_acquire_ctx *ww_ctx)

Please update the kerneldoc to talk about the return value.


>  {
>         struct regulator_dev *held, *contended;
>         int ret;
> @@ -208,10 +208,13 @@ static void regulator_lock_two(struct regulator_dev *rdev1,
>
>         /* Try to just grab both of them */
>         ret = regulator_lock_nested(rdev1, ww_ctx);
> -       WARN_ON(ret);
> +       if (WARN_ON(ret))
> +               goto exit;
>         ret = regulator_lock_nested(rdev2, ww_ctx);
> -       if (ret != -EDEADLOCK) {
> -               WARN_ON(ret);
> +       if (!ret)
> +               return 0;

Aren't you missing the "ww_acquire_done()" in this case? I know it's
optional, but nice to call it anyway...


> +       if (WARN_ON(ret != -EDEADLOCK)) {
> +               regulator_unlock(rdev1);
>                 goto exit;
>         }
>
> @@ -225,15 +228,15 @@ static void regulator_lock_two(struct regulator_dev *rdev1,
>                 contended->mutex_owner = current;
>                 swap(held, contended);
>                 ret = regulator_lock_nested(contended, ww_ctx);
> -
> -               if (ret != -EDEADLOCK) {
> -                       WARN_ON(ret);
> +               if (!ret)
> +                       return 0;

Aren't you missing the "ww_acquire_done()" in this case?


Overall, I guess I'm OK with this. I agree that we'd be in pretty bad
shape if we exited the routine and left one of the mutexes acquired,
which the old code could have done. However, at the same time these
error cases really shouldn't happen anyway. In that sense, I guess
they really should be BUG_ON(), but IIRC that's discouraged. Thus, I
think your patch is the right direction.


-Doug