On 21/05/18 06:48 PM, Jon Mason wrote: > On Fri, Mar 09, 2018 at 04:03:24PM +0530, Arvind Yadav wrote: >> Never directly free @dev after calling device_register(), even >> if it returned an error! Always use put_device() to give up the >> reference initialized. >> >> Signed-off-by: Arvind Yadav <[email protected]> >> --- >> drivers/ntb/ntb_transport.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c >> index 9878c48..8182a3a 100644 >> --- a/drivers/ntb/ntb_transport.c >> +++ b/drivers/ntb/ntb_transport.c >> @@ -393,7 +393,7 @@ int ntb_transport_register_client_dev(char *device_name) >> >> rc = device_register(dev); >> if (rc) { >> - kfree(client_dev); >> + put_device(dev); > > Now we are leaking client_dev, which is bigger than just dev. I think > we are going to need both now.
No, when put_device is called, ntb_transport_client_release() will be called which then kfree's the structure there. This is the preferred way of freeing anything that's reference counted (as all struct devices are). See [1]. Though, if I remember correctly, the NTB tree breaks this rule a lot. ie. the fact that ntb_dev_release() doesn't actually free the underlying structure has always irked me a bit because it forces the calling drivers to break this rule. This means the reference counting on the NTB devices is broken so if someone ever uses get_device() on an struct ntb_dev and doesn't put it back before the driver is unbound, there will be a use after free bug. As far as I know though, at this time this isn't done. Logan [1] https://elixir.bootlin.com/linux/v4.17-rc6/source/drivers/base/core.c#L1931
