linux-kernel - Re: [PATCH 2/3] gpiolib: use kref in gpio

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: <CACRpkdYpers8Zzh9A3T0mFSyZYDcrjfn9iaQn92RkVHWE+GinQ@mail.gmail.com>
Date:   Fri, 13 Mar 2020 09:43:56 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     Bartosz Golaszewski <brgl@...ev.pl>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Khouloud Touil <ktouil@...libre.com>
Subject: Re: [PATCH 2/3] gpiolib: use kref in gpio_desc

On Thu, Mar 12, 2020 at 7:25 PM Bartosz Golaszewski
<bgolaszewski@...libre.com> wrote:

> I believe this is not correct. The resources managed by devres are
> released when the device is detached from a driver, not when the
> device's reference count goes to 0. When the latter happens, the
> device's specific (or its device_type's) release callback is called -
> for gpiolib this is gpiodevice_release().

Yeah you're right, I even point that out in my second letter :/

It's a bit of confusion for everyone (or it's just me).

> The kref inside struct device will not go down to zero until you call
> device_del() (if you previously called device_add() that is which
> increases the reference count by a couple points). But what I'm
> thinking about is making the call to device_del() depend not on the
> call to gpiochip_remove() but on the kref on the gpio device going
> down to zero. As for the protection against module removal - this
> should be handled by module_get/put().

Right. At the end of gpiochip_remove():

   cdev_device_del(&gdev->chrdev, &gdev->dev);
   put_device(&gdev->dev);

That last put_device() should in best case bring the refcount
to zero.

So the actual way we lifecycle GPIO chips is managed
resources using only devm_* but the reference count does work
too: reference count should normally land at zero since the
gpiochip_remove() call is ended with a call to
put_device() and that should (ideally) bring it to zero.

It's just that this doesn't really trigger anything.

I think there is no way out of the fact that we have to
forcefully remove the gpio_chip when devm_* destructors
kicks in: the driver is indeed getting removed at that
point.

In gpiochip_remove() we "numb" the chip so that any
gpio_desc:s currently in use will just fail silently and not crash,
since they are not backed by a driver any more. The descs
stay around until the consumer releases them, but if we probe the
same GPIO device again they will certainly not re-attach or
something.

Arguably it is a bit of policy. Would it make more sense to
have rmmod fail if the kref inside gdev->dev->kobj->kref
is != 1? I suppose that is what things like storage
drivers pretty much have to do.

The problem with that is that as soon as you have a consumer
that is compiled into the kernel it makes it impossible to
remove the gpio driver with rmmod.

I really needed to refresh this a bit, so the above is maybe
a bit therapeutic.

I don't really see how we could do things differently without
creating some other problem though.

Yours,
Linus Walleij