linux-kernel - Re: [PATCH] iio: light: gp2ap002: Fix rumtime PM imbalance on error

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: <20210412111506.0000653c@Huawei.com>
Date:   Mon, 12 Apr 2021 11:15:06 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Dinghao Liu <dinghao.liu@....edu.cn>,
        Kangjie Lu <kjlu@....edu>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Peter Meerwald-Stadler" <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: light: gp2ap002: Fix rumtime PM imbalance on error

On Mon, 12 Apr 2021 00:38:41 +0200
Linus Walleij <linus.walleij@...aro.org> wrote:

> On Sun, Apr 11, 2021 at 5:07 PM Jonathan Cameron <jic23@...nel.org> wrote:
> > On Wed,  7 Apr 2021 11:49:27 +0800
> > Dinghao Liu <dinghao.liu@....edu.cn> wrote:
> >  
> > > When devm_request_threaded_irq() fails, we should decrease the
> > > runtime PM counter to keep the counter balanced. But when
> > > iio_device_register() fails, we need not to decrease it because
> > > we have already decreased it before.  
> >
> > Whilst agree with your assessment that the code is wrong, I'm not
> > totally sure why we need to do the pm_runtime_get_noresume() in
> > the first place.   Why do we need to hold the reference for
> > the operations going on here?  What can race against this that
> > might care about that reference count?  
> 
> pm_runtime_get_noresume() is increasing the runtime PM
> reference without calling the pm_runtime_resume() callback.
> 
> It is often called in sequence like this:
> 
>     pm_runtime_get_noresume(dev);
>     pm_runtime_set_active(dev);
>     pm_runtime_enable(dev);
> 
> This increases the reference, sets the device as active
> and enables runtime PM.
> 
> The reason that probe() has activated resources such as
> enabling two regulators, and want to leave them on so that
> later on pm_runtime_suspend() will disable them, i.e.
> handover to runtime PM with the device in resumed state.
> 
> I hope this is answering the question, not sure.

There are drivers that look the same except they aren't
holding the reference.  Are those immediately disabling the power?
I can't see the path by which that happens, but perhaps I'm just
missing something?   Maybe this is just paranoid locking in
a probe path (before we are in a position where races can occur)?

An example would be the bmc150_magn driver which does exactly the
same call sequence as this one, but without the reference count increment
and decrement.  Basically I want to know if there is a problem in
those other drivers that is being protected against here!

> 
> Yours,
> Linus Walleij