Re: [PATCH] nvmem: fix unregistering device in nvmem_register() error path

[Johan Hovold <johan@kernel.org>]

On Wed, Dec 22, 2021 at 10:03:17AM +0100, Johan Hovold wrote:
> On Wed, Dec 22, 2021 at 09:56:29AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Dec 22, 2021 at 09:38:27AM +0100, 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@milecki.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.
> > 
> > A short search found almost everyone getting this wrong.  Arguably
> > platform_device_register() can clean up properly on its own if we want
> > it to do so.  Will take a lot of auditing of the current codebase first
> > to see if it's safe...
> 
> Right, but I found at least a couple of callers getting it it right, so
> changing the behaviour now risks introducing a double free (which is
> worse than a memleak on registration failure). But yeah, a careful
> review might suffice.

Actually, I'm not sure we can (should) change
platform_device_register(). The platform device has been allocated by
the caller and it would be quite counterintuitive to have the
registration function deallocate that memory if registration fails.

Heh, we even have statically allocated structures being registered with
this function and we certainly don't want the helper to try to free
those.

Johan