linux-kernel - Re: [PATCH] nvmem: fix unregistering device in nvmem

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: <YcLvY5tJJcxusM0a@hovoldconsulting.com>
Date:   Wed, 22 Dec 2021 10:26:59 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Rafał Miłecki <rafal@...ecki.pl>
Cc:     Rafał Miłecki <zajec5@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Andrey Smirnov <andrew.smirnov@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nvmem: fix unregistering device in nvmem_register()
 error path

On Wed, Dec 22, 2021 at 10:16:20AM +0100, Rafał Miłecki wrote:
> On 22.12.2021 10:08, Johan Hovold wrote:
> > On Wed, Dec 22, 2021 at 10:00:03AM +0100, Rafał Miłecki wrote:
> >> On 22.12.2021 09:38, Johan Hovold wrote:
> > 
> >>> It seems Rafał is mistaken here too; you certainly need to call
> >>> platform_device_put() if platform_device_register() fail, even if many
> >>> current users do appear to get this wrong.
> >>
> >> Yes I was! Gosh I made up that "platform_device_put()" name and only
> >> now I realized it actually exists!
> >>
> >> I stand by saying this design is really misleading. Even though
> >> platform_device_put() was obviously a bad example.
> >>
> >> Please remember I'm just a minor kernel developer however in my humble
> >> opinion behaviour of device_register() and platform_device_register()
> >> should be changed.
> >>
> >> If any function fails I expect:
> >> 1. That function to clean up its mess if any
> >> 2. Me to be responsible to clean up my mess if any
> >>
> >> This is how "most" code (whatever it means) works.
> >> 1. If POSIX snprintf() fails I'm not expected to call *printf_put() sth
> >> 2. If POSIX bind() fails I'm not expected to call bind_put() sth
> >> 3. (...)
> >>
> >> I'm not sure if those are the best examples but you should get my point.
> > 
> > Yes, and we all agree that it's not the best interface. But it exists,
> > and changing it now risks introducing worse problem than a minor, mostly
> > theoretical, memleak.
> 
> Thanks for confirming that, I was wondering if it's just my mind that
> doesn't find this design clear enough.
> 
> Now, assuming this design isn't perfect and some purists would like it
> cleaned up:
> 
> Would that make sense to introduce something like
> 1. device_register2() / device_add2()
> and
> 2. platform_device_register2() / platform_device_add2()
> 
> that would *not* require calling *_put() on failure? Then start
> converting existing drivers to those new (clearner?) helpers?

Nah, let's not add more helpers. Also see my last reply to Greg about
why the registration helper cannot free object being registered.

device_initialize() is special, and everyone just needs to learn that.

Johan