linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] thunderbolt: Fix to check return value of ida_simple_get
@ 2019-03-20 16:24 Aditya Pakki
  2019-03-20 16:29 ` Mika Westerberg
  0 siblings, 1 reply; 5+ messages in thread
From: Aditya Pakki @ 2019-03-20 16:24 UTC (permalink / raw)
  To: pakki001
  Cc: kjlu, Andreas Noever, Michael Jamet, Mika Westerberg,
	Yehezkel Bernat, linux-kernel

In enumerate_services, ida_simple_get on failure can return an error and
leaks memory during device_register failure. The patch ensures that
the dev_set_name is set on non failure cases, and releases memory in
case of failure.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>

---
v1: Missed cleanup of svc in case of allocation failure and
device_register failure.
---
 drivers/thunderbolt/xdomain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index e27dd8beb94b..eb08275185bf 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -740,6 +740,7 @@ static void enumerate_services(struct tb_xdomain *xd)
 	struct tb_service *svc;
 	struct tb_property *p;
 	struct device *dev;
+	int id;
 
 	/*
 	 * First remove all services that are not available anymore in
@@ -768,7 +769,12 @@ static void enumerate_services(struct tb_xdomain *xd)
 			break;
 		}
 
-		svc->id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
+		id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
+		if (id < 0) {
+			kfree(svc);
+			break;
+		}
+		svc->id = id;
 		svc->dev.bus = &tb_bus_type;
 		svc->dev.type = &tb_service_type;
 		svc->dev.parent = &xd->dev;
@@ -776,6 +782,7 @@ static void enumerate_services(struct tb_xdomain *xd)
 
 		if (device_register(&svc->dev)) {
 			put_device(&svc->dev);
+			kfree(svc);
 			break;
 		}
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] thunderbolt: Fix to check return value of ida_simple_get
  2019-03-20 16:24 [PATCH v2] thunderbolt: Fix to check return value of ida_simple_get Aditya Pakki
@ 2019-03-20 16:29 ` Mika Westerberg
  2019-03-20 20:39   ` Mukesh Ojha
  0 siblings, 1 reply; 5+ messages in thread
From: Mika Westerberg @ 2019-03-20 16:29 UTC (permalink / raw)
  To: Aditya Pakki
  Cc: kjlu, Andreas Noever, Michael Jamet, Yehezkel Bernat, linux-kernel

On Wed, Mar 20, 2019 at 11:24:45AM -0500, Aditya Pakki wrote:
> In enumerate_services, ida_simple_get on failure can return an error and
> leaks memory during device_register failure. The patch ensures that
> the dev_set_name is set on non failure cases, and releases memory in
> case of failure.
> 
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> 
> ---
> v1: Missed cleanup of svc in case of allocation failure and
> device_register failure.
> ---
>  drivers/thunderbolt/xdomain.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
> index e27dd8beb94b..eb08275185bf 100644
> --- a/drivers/thunderbolt/xdomain.c
> +++ b/drivers/thunderbolt/xdomain.c
> @@ -740,6 +740,7 @@ static void enumerate_services(struct tb_xdomain *xd)
>  	struct tb_service *svc;
>  	struct tb_property *p;
>  	struct device *dev;
> +	int id;
>  
>  	/*
>  	 * First remove all services that are not available anymore in
> @@ -768,7 +769,12 @@ static void enumerate_services(struct tb_xdomain *xd)
>  			break;
>  		}
>  
> -		svc->id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
> +		id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
> +		if (id < 0) {
> +			kfree(svc);
> +			break;
> +		}
> +		svc->id = id;
>  		svc->dev.bus = &tb_bus_type;
>  		svc->dev.type = &tb_service_type;
>  		svc->dev.parent = &xd->dev;
> @@ -776,6 +782,7 @@ static void enumerate_services(struct tb_xdomain *xd)
>  
>  		if (device_register(&svc->dev)) {
>  			put_device(&svc->dev);
> +			kfree(svc);

You can't do this after device_register() is called. The put_device()
above is sufficient.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] thunderbolt: Fix to check return value of ida_simple_get
  2019-03-20 16:29 ` Mika Westerberg
@ 2019-03-20 20:39   ` Mukesh Ojha
  2019-03-21  5:49     ` Mika Westerberg
  0 siblings, 1 reply; 5+ messages in thread
From: Mukesh Ojha @ 2019-03-20 20:39 UTC (permalink / raw)
  To: Mika Westerberg, Aditya Pakki
  Cc: kjlu, Andreas Noever, Michael Jamet, Yehezkel Bernat, linux-kernel


On 3/20/2019 9:59 PM, Mika Westerberg wrote:
> On Wed, Mar 20, 2019 at 11:24:45AM -0500, Aditya Pakki wrote:
>> In enumerate_services, ida_simple_get on failure can return an error and
>> leaks memory during device_register failure. The patch ensures that
>> the dev_set_name is set on non failure cases, and releases memory in
>> case of failure.
>>
>> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
>>
>> ---
>> v1: Missed cleanup of svc in case of allocation failure and
>> device_register failure.
>> ---
>>   drivers/thunderbolt/xdomain.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
>> index e27dd8beb94b..eb08275185bf 100644
>> --- a/drivers/thunderbolt/xdomain.c
>> +++ b/drivers/thunderbolt/xdomain.c
>> @@ -740,6 +740,7 @@ static void enumerate_services(struct tb_xdomain *xd)
>>   	struct tb_service *svc;
>>   	struct tb_property *p;
>>   	struct device *dev;
>> +	int id;
>>   
>>   	/*
>>   	 * First remove all services that are not available anymore in
>> @@ -768,7 +769,12 @@ static void enumerate_services(struct tb_xdomain *xd)
>>   			break;
>>   		}
>>   
>> -		svc->id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
>> +		id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
>> +		if (id < 0) {
>> +			kfree(svc);
>> +			break;
>> +		}
>> +		svc->id = id;
>>   		svc->dev.bus = &tb_bus_type;
>>   		svc->dev.type = &tb_service_type;
>>   		svc->dev.parent = &xd->dev;
>> @@ -776,6 +782,7 @@ static void enumerate_services(struct tb_xdomain *xd)
>>   
>>   		if (device_register(&svc->dev)) {
>>   			put_device(&svc->dev);
>> +			kfree(svc);
> You can't do this after device_register() is called. The put_device()
> above is sufficient.


If  device_register fails, how would svc gets freed? we need  to kfree 
svc here as well.

Thanks,
Mukesh


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] thunderbolt: Fix to check return value of ida_simple_get
  2019-03-20 20:39   ` Mukesh Ojha
@ 2019-03-21  5:49     ` Mika Westerberg
  2019-03-22  7:31       ` Mukesh Ojha
  0 siblings, 1 reply; 5+ messages in thread
From: Mika Westerberg @ 2019-03-21  5:49 UTC (permalink / raw)
  To: Mukesh Ojha
  Cc: Aditya Pakki, kjlu, Andreas Noever, Michael Jamet,
	Yehezkel Bernat, linux-kernel

On Thu, Mar 21, 2019 at 02:09:41AM +0530, Mukesh Ojha wrote:
> 
> On 3/20/2019 9:59 PM, Mika Westerberg wrote:
> > On Wed, Mar 20, 2019 at 11:24:45AM -0500, Aditya Pakki wrote:
> > > In enumerate_services, ida_simple_get on failure can return an error and
> > > leaks memory during device_register failure. The patch ensures that
> > > the dev_set_name is set on non failure cases, and releases memory in
> > > case of failure.
> > > 
> > > Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> > > 
> > > ---
> > > v1: Missed cleanup of svc in case of allocation failure and
> > > device_register failure.
> > > ---
> > >   drivers/thunderbolt/xdomain.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
> > > index e27dd8beb94b..eb08275185bf 100644
> > > --- a/drivers/thunderbolt/xdomain.c
> > > +++ b/drivers/thunderbolt/xdomain.c
> > > @@ -740,6 +740,7 @@ static void enumerate_services(struct tb_xdomain *xd)
> > >   	struct tb_service *svc;
> > >   	struct tb_property *p;
> > >   	struct device *dev;
> > > +	int id;
> > >   	/*
> > >   	 * First remove all services that are not available anymore in
> > > @@ -768,7 +769,12 @@ static void enumerate_services(struct tb_xdomain *xd)
> > >   			break;
> > >   		}
> > > -		svc->id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
> > > +		id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
> > > +		if (id < 0) {
> > > +			kfree(svc);
> > > +			break;
> > > +		}
> > > +		svc->id = id;
> > >   		svc->dev.bus = &tb_bus_type;
> > >   		svc->dev.type = &tb_service_type;
> > >   		svc->dev.parent = &xd->dev;
> > > @@ -776,6 +782,7 @@ static void enumerate_services(struct tb_xdomain *xd)
> > >   		if (device_register(&svc->dev)) {
> > >   			put_device(&svc->dev);
> > > +			kfree(svc);
> > You can't do this after device_register() is called. The put_device()
> > above is sufficient.
> 
> 
> If  device_register fails, how would svc gets freed? we need  to kfree svc
> here as well.

Please read the comment on top of device_register(). It should explain.

So no kfree here.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] thunderbolt: Fix to check return value of ida_simple_get
  2019-03-21  5:49     ` Mika Westerberg
@ 2019-03-22  7:31       ` Mukesh Ojha
  0 siblings, 0 replies; 5+ messages in thread
From: Mukesh Ojha @ 2019-03-22  7:31 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Aditya Pakki, kjlu, Andreas Noever, Michael Jamet,
	Yehezkel Bernat, linux-kernel


On 3/21/2019 11:19 AM, Mika Westerberg wrote:
> On Thu, Mar 21, 2019 at 02:09:41AM +0530, Mukesh Ojha wrote:
>> On 3/20/2019 9:59 PM, Mika Westerberg wrote:
>>> On Wed, Mar 20, 2019 at 11:24:45AM -0500, Aditya Pakki wrote:
>>>> In enumerate_services, ida_simple_get on failure can return an error and
>>>> leaks memory during device_register failure. The patch ensures that
>>>> the dev_set_name is set on non failure cases, and releases memory in
>>>> case of failure.
>>>>
>>>> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
>>>>
>>>> ---
>>>> v1: Missed cleanup of svc in case of allocation failure and
>>>> device_register failure.
>>>> ---
>>>>    drivers/thunderbolt/xdomain.c | 9 ++++++++-
>>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
>>>> index e27dd8beb94b..eb08275185bf 100644
>>>> --- a/drivers/thunderbolt/xdomain.c
>>>> +++ b/drivers/thunderbolt/xdomain.c
>>>> @@ -740,6 +740,7 @@ static void enumerate_services(struct tb_xdomain *xd)
>>>>    	struct tb_service *svc;
>>>>    	struct tb_property *p;
>>>>    	struct device *dev;
>>>> +	int id;
>>>>    	/*
>>>>    	 * First remove all services that are not available anymore in
>>>> @@ -768,7 +769,12 @@ static void enumerate_services(struct tb_xdomain *xd)
>>>>    			break;
>>>>    		}
>>>> -		svc->id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
>>>> +		id = ida_simple_get(&xd->service_ids, 0, 0, GFP_KERNEL);
>>>> +		if (id < 0) {
>>>> +			kfree(svc);
>>>> +			break;
>>>> +		}
>>>> +		svc->id = id;
>>>>    		svc->dev.bus = &tb_bus_type;
>>>>    		svc->dev.type = &tb_service_type;
>>>>    		svc->dev.parent = &xd->dev;
>>>> @@ -776,6 +782,7 @@ static void enumerate_services(struct tb_xdomain *xd)
>>>>    		if (device_register(&svc->dev)) {
>>>>    			put_device(&svc->dev);
>>>> +			kfree(svc);
>>> You can't do this after device_register() is called. The put_device()
>>> above is sufficient.
>>
>> If  device_register fails, how would svc gets freed? we need  to kfree svc
>> here as well.
> Please read the comment on top of device_register(). It should explain.
>
> So no kfree here.

Thanks for pointer Mika.

Overlooked the fact that dev is a data member of svc not a pointer, also 
noticed there are many places in tree where , it is not
followed.

Thanks,
Mukesh


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-03-22  7:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-20 16:24 [PATCH v2] thunderbolt: Fix to check return value of ida_simple_get Aditya Pakki
2019-03-20 16:29 ` Mika Westerberg
2019-03-20 20:39   ` Mukesh Ojha
2019-03-21  5:49     ` Mika Westerberg
2019-03-22  7:31       ` Mukesh Ojha

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).