Re: [PATCH] ntb_transport: use put_device() instead of kfree()
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>> --- >> 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
Re: [PATCH] ntb_transport: use put_device() instead of kfree()
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 >> --- >> 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
Re: [PATCH] ntb_transport: use put_device() instead of kfree()
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> --- > 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. Thanks, Jon > goto err; > } > > -- > 1.9.1 >
Re: [PATCH] ntb_transport: use put_device() instead of kfree()
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 > --- > 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. Thanks, Jon > goto err; > } > > -- > 1.9.1 >
Re: [PATCH] ntb_transport: use put_device() instead of kfree()
Looks correct to me. Thanks! Reviewed-by: Logan GunthorpeOn 09/03/18 03:33 AM, 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 --- 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); goto err; }
Re: [PATCH] ntb_transport: use put_device() instead of kfree()
Looks correct to me. Thanks! Reviewed-by: Logan Gunthorpe On 09/03/18 03:33 AM, 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 --- 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); goto err; }
[PATCH] ntb_transport: use put_device() instead of kfree()
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--- 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); goto err; } -- 1.9.1
[PATCH] ntb_transport: use put_device() instead of kfree()
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 --- 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); goto err; } -- 1.9.1