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]

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

On 22.12.2021 09:38, Johan Hovold wrote:
> On Wed, Dec 22, 2021 at 08:44:44AM +0100, Greg Kroah-Hartman wrote:
>> On Tue, Dec 21, 2021 at 06:46:01PM +0100, Rafał Miłecki wrote:
>>> On 21.12.2021 17:06, Greg Kroah-Hartman wrote:
>>>> On Tue, Dec 21, 2021 at 04:45:50PM +0100, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@...ecki.pl>
>>>>>
>>>>> 1. Drop incorrect put_device() calls
>>>>>
>>>>> If device_register() fails then underlaying device_add() takes care of
>>>>> calling put_device() if needed. There is no need to do that in a driver.
>>>>
>>>> Did you read the documentation for device_register() that says:
>>>>
>>>>    * NOTE: _Never_ directly free @dev after calling this function, even
>>>>    * if it returned an error! Always use put_device() to give up the
>>>>    * reference initialized in this function instead.
>>>
>>> I clearly tried to be too smart and ignored documentation.
>>>
>>> I'd say device_add() behaviour is rather uncommon and a bit unintuitive.
>>> Most kernel functions are safe to assume to do nothing that requires
>>> cleanup if they fail.
>>>
>>> E.g. if I call platform_device_register() and it fails I don't need to
>>> call anything like platform_device_put(). I just free previously
>>> allocated memory.
>>
>> And that is wrong.
> 
> 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.