* [PATCH] drivers/acpi/processor_driver.c: add missing kfree
@ 2012-03-10 17:05 Julia Lawall
2012-03-14 11:34 ` Srivatsa S. Bhat
0 siblings, 1 reply; 9+ messages in thread
From: Julia Lawall @ 2012-03-10 17:05 UTC (permalink / raw)
To: Len Brown; +Cc: kernel-janitors, linux-acpi, linux-kernel, Julia Lawall
From: Julia Lawall <julia@diku.dk>
The function acpi_processor_add is stored in the ops.add field of a
acpi_driver structure. This function is then called in
acpi_bus_driver_init. On failure, this function clears the field
device->driver_data, but does not free its contents. Thus the free has to
be done by the add function. In acpi_processor_add, the corresponding
value is pr. This value is currently freed on failure before storing it in
device->driver_data, but not after. This free is added in the error
handling code at the end of the function. The static global variable
processors is also cleared so that it does not refer to a dangling pointer.
processor_device_array is cleared as well.
Signed-off-by: Julia Lawall <julia@diku.dk>
---
This is only compile tested. In particular, I don't know if it is correct
to add per_cpu(processor_device_array, pr->id) = NULL;. It has nothing to
do with pr, but it looks like stale information in the case of a failure.
drivers/acpi/processor_driver.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 2801b41..9bb0017 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
return -ENOMEM;
if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
- kfree(pr);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_free_pr;
}
pr->handle = device->handle;
@@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
dev = get_cpu_device(pr->id);
if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
result = -EFAULT;
- goto err_free_cpumask;
+ goto err_clear_processors;
}
/*
@@ -595,9 +595,13 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
err_remove_sysfs:
sysfs_remove_link(&device->dev.kobj, "sysdev");
+err_clear_processors:
+ per_cpu(processors, pr->id) = NULL;
+ per_cpu(processor_device_array, pr->id) = NULL;
err_free_cpumask:
free_cpumask_var(pr->throttling.shared_cpu_map);
-
+err_free_pr:
+ kfree(pr);
return result;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-10 17:05 Julia Lawall
@ 2012-03-14 11:34 ` Srivatsa S. Bhat
2012-03-14 12:32 ` Julia Lawall
0 siblings, 1 reply; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-03-14 11:34 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, kernel-janitors, linux-acpi, linux-kernel,
Julia Lawall, Deepthi Dharwar, Venkatesh Pallipadi, trenn,
bhelgaas
On 03/10/2012 10:35 PM, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
>
> The function acpi_processor_add is stored in the ops.add field of a
> acpi_driver structure. This function is then called in
> acpi_bus_driver_init. On failure, this function clears the field
> device->driver_data, but does not free its contents. Thus the free has to
> be done by the add function. In acpi_processor_add, the corresponding
> value is pr. This value is currently freed on failure before storing it in
> device->driver_data, but not after. This free is added in the error
> handling code at the end of the function. The static global variable
"static global variable"?? never heard that one before ;-)
Maybe you meant "per_cpu variable processors"..
> processors is also cleared so that it does not refer to a dangling pointer.
> processor_device_array is cleared as well.
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> This is only compile tested. In particular, I don't know if it is correct
> to add per_cpu(processor_device_array, pr->id) = NULL;.
No, you shouldn't set it to NULL. processor_device_array was added to check
for buggy BIOSes and return gracefully. Check commit cd8e2b48d (and also the
bugzilla link in that commit).
To have a robust check for buggy BIOSes, we must let it be as it is, even if
it is a stale pointer. That way we can still catch subsequent calls to this
function with the same acpi id (because of a buggy BIOS) and take appropriate
actions.
Other than that, the patch looks good to me.
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
> It has nothing to
> do with pr, but it looks like stale information in the case of a failure.
>
> drivers/acpi/processor_driver.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 2801b41..9bb0017 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> return -ENOMEM;
>
> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
> - kfree(pr);
> - return -ENOMEM;
> + result = -ENOMEM;
> + goto err_free_pr;
> }
>
> pr->handle = device->handle;
> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> dev = get_cpu_device(pr->id);
> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
> result = -EFAULT;
> - goto err_free_cpumask;
> + goto err_clear_processors;
> }
>
> /*
> @@ -595,9 +595,13 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>
> err_remove_sysfs:
> sysfs_remove_link(&device->dev.kobj, "sysdev");
> +err_clear_processors:
> + per_cpu(processors, pr->id) = NULL;
> + per_cpu(processor_device_array, pr->id) = NULL;
> err_free_cpumask:
> free_cpumask_var(pr->throttling.shared_cpu_map);
> -
> +err_free_pr:
> + kfree(pr);
> return result;
> }
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-14 11:34 ` Srivatsa S. Bhat
@ 2012-03-14 12:32 ` Julia Lawall
2012-03-14 12:50 ` Srivatsa S. Bhat
0 siblings, 1 reply; 9+ messages in thread
From: Julia Lawall @ 2012-03-14 12:32 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Julia Lawall, Len Brown, kernel-janitors, linux-acpi,
linux-kernel, Julia Lawall, Deepthi Dharwar, Venkatesh Pallipadi,
trenn, bhelgaas
On Wed, 14 Mar 2012, Srivatsa S. Bhat wrote:
> On 03/10/2012 10:35 PM, Julia Lawall wrote:
>
>> From: Julia Lawall <julia@diku.dk>
>>
>> The function acpi_processor_add is stored in the ops.add field of a
>> acpi_driver structure. This function is then called in
>> acpi_bus_driver_init. On failure, this function clears the field
>> device->driver_data, but does not free its contents. Thus the free has to
>> be done by the add function. In acpi_processor_add, the corresponding
>> value is pr. This value is currently freed on failure before storing it in
>> device->driver_data, but not after. This free is added in the error
>> handling code at the end of the function. The static global variable
>
>
> "static global variable"?? never heard that one before ;-)
> Maybe you meant "per_cpu variable processors"..
>
>> processors is also cleared so that it does not refer to a dangling pointer.
>> processor_device_array is cleared as well.
>>
>> Signed-off-by: Julia Lawall <julia@diku.dk>
>>
>> ---
>> This is only compile tested. In particular, I don't know if it is correct
>> to add per_cpu(processor_device_array, pr->id) = NULL;.
>
>
> No, you shouldn't set it to NULL. processor_device_array was added to check
> for buggy BIOSes and return gracefully. Check commit cd8e2b48d (and also the
> bugzilla link in that commit).
>
> To have a robust check for buggy BIOSes, we must let it be as it is, even if
> it is a stale pointer. That way we can still catch subsequent calls to this
> function with the same acpi id (because of a buggy BIOS) and take appropriate
> actions.
>
> Other than that, the patch looks good to me.
Thanks for the feedback. Just to be clear, I should keep
+err_clear_processors:
+ per_cpu(processors, pr->id) = NULL;
and just drop:
+ per_cpu(processor_device_array, pr->id) = NULL;
julia
>
> Regards,
> Srivatsa S. Bhat
> IBM Linux Technology Center
>
>> It has nothing to
>> do with pr, but it looks like stale information in the case of a failure.
>>
>> drivers/acpi/processor_driver.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 2801b41..9bb0017 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>> return -ENOMEM;
>>
>> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
>> - kfree(pr);
>> - return -ENOMEM;
>> + result = -ENOMEM;
>> + goto err_free_pr;
>> }
>>
>> pr->handle = device->handle;
>> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>> dev = get_cpu_device(pr->id);
>> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
>> result = -EFAULT;
>> - goto err_free_cpumask;
>> + goto err_clear_processors;
>> }
>>
>> /*
>> @@ -595,9 +595,13 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>>
>> err_remove_sysfs:
>> sysfs_remove_link(&device->dev.kobj, "sysdev");
>> +err_clear_processors:
>> + per_cpu(processors, pr->id) = NULL;
>> + per_cpu(processor_device_array, pr->id) = NULL;
>> err_free_cpumask:
>> free_cpumask_var(pr->throttling.shared_cpu_map);
>> -
>> +err_free_pr:
>> + kfree(pr);
>> return result;
>> }
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-14 12:32 ` Julia Lawall
@ 2012-03-14 12:50 ` Srivatsa S. Bhat
0 siblings, 0 replies; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-03-14 12:50 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, kernel-janitors, linux-acpi, linux-kernel,
Julia Lawall, Deepthi Dharwar, Venkatesh Pallipadi, trenn,
bhelgaas
On 03/14/2012 06:02 PM, Julia Lawall wrote:
>
>
> On Wed, 14 Mar 2012, Srivatsa S. Bhat wrote:
>
>> On 03/10/2012 10:35 PM, Julia Lawall wrote:
>>
>>> From: Julia Lawall <julia@diku.dk>
>>>
>>> The function acpi_processor_add is stored in the ops.add field of a
>>> acpi_driver structure. This function is then called in
>>> acpi_bus_driver_init. On failure, this function clears the field
>>> device->driver_data, but does not free its contents. Thus the free
>>> has to
>>> be done by the add function. In acpi_processor_add, the corresponding
>>> value is pr. This value is currently freed on failure before storing
>>> it in
>>> device->driver_data, but not after. This free is added in the error
>>> handling code at the end of the function. The static global variable
>>
>>
>> "static global variable"?? never heard that one before ;-)
>> Maybe you meant "per_cpu variable processors"..
>>
>>> processors is also cleared so that it does not refer to a dangling
>>> pointer.
>>> processor_device_array is cleared as well.
>>>
>>> Signed-off-by: Julia Lawall <julia@diku.dk>
>>>
>>> ---
>>> This is only compile tested. In particular, I don't know if it is
>>> correct
>>> to add per_cpu(processor_device_array, pr->id) = NULL;.
>>
>>
>> No, you shouldn't set it to NULL. processor_device_array was added to
>> check
>> for buggy BIOSes and return gracefully. Check commit cd8e2b48d (and
>> also the
>> bugzilla link in that commit).
>>
>> To have a robust check for buggy BIOSes, we must let it be as it is,
>> even if
>> it is a stale pointer. That way we can still catch subsequent calls to
>> this
>> function with the same acpi id (because of a buggy BIOS) and take
>> appropriate
>> actions.
>>
>> Other than that, the patch looks good to me.
>
> Thanks for the feedback. Just to be clear, I should keep
>
> +err_clear_processors:
> + per_cpu(processors, pr->id) = NULL;
>
> and just drop:
>
> + per_cpu(processor_device_array, pr->id) = NULL;
>
Yep, that's right. But in fact, you can even do better than that...
That is, drop the above line and put a comment that explains why we
shouldn't set per_cpu(processor_device_array..) to NULL. That way,
in future people will know that setting it to NULL was left out on
purpose.
And please adjust the commit message too, as I pointed in my previous
mail :-)
Regards,
Srivatsa S. Bhat
>>>
>>> drivers/acpi/processor_driver.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/acpi/processor_driver.c
>>> b/drivers/acpi/processor_driver.c
>>> index 2801b41..9bb0017 100644
>>> --- a/drivers/acpi/processor_driver.c
>>> +++ b/drivers/acpi/processor_driver.c
>>> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct
>>> acpi_device *device)
>>> return -ENOMEM;
>>>
>>> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map,
>>> GFP_KERNEL)) {
>>> - kfree(pr);
>>> - return -ENOMEM;
>>> + result = -ENOMEM;
>>> + goto err_free_pr;
>>> }
>>>
>>> pr->handle = device->handle;
>>> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct
>>> acpi_device *device)
>>> dev = get_cpu_device(pr->id);
>>> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
>>> result = -EFAULT;
>>> - goto err_free_cpumask;
>>> + goto err_clear_processors;
>>> }
>>>
>>> /*
>>> @@ -595,9 +595,13 @@ static int __cpuinit acpi_processor_add(struct
>>> acpi_device *device)
>>>
>>> err_remove_sysfs:
>>> sysfs_remove_link(&device->dev.kobj, "sysdev");
>>> +err_clear_processors:
>>> + per_cpu(processors, pr->id) = NULL;
>>> + per_cpu(processor_device_array, pr->id) = NULL;
>>> err_free_cpumask:
>>> free_cpumask_var(pr->throttling.shared_cpu_map);
>>> -
>>> +err_free_pr:
>>> + kfree(pr);
>>> return result;
>>> }
>>>
>>>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drivers/acpi/processor_driver.c: add missing kfree
@ 2012-03-15 8:32 Julia Lawall
2012-03-15 9:07 ` Srivatsa S. Bhat
2012-03-15 9:32 ` Deepthi Dharwar
0 siblings, 2 replies; 9+ messages in thread
From: Julia Lawall @ 2012-03-15 8:32 UTC (permalink / raw)
To: Len Brown, srivatsa.bhat, deepthi, venki, trenn, bhelgaas
Cc: kernel-janitors, linux-acpi, linux-kernel
From: Julia Lawall <Julia.Lawall@lip6.fr>
The function acpi_processor_add is stored in the ops.add field of a
acpi_driver structure. This function is then called in
acpi_bus_driver_init. On failure, this function clears the field
device->driver_data, but does not free its contents. Thus the free has to
be done by the add function. In acpi_processor_add, the corresponding
value is pr. This value is currently freed on failure before storing it in
device->driver_data, but not after. This free is added in the error
handling code at the end of the function. The per_cpu variable
processors is also cleared so that it does not refer to a dangling pointer.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
drivers/acpi/processor_driver.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 2801b41..98c3648 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
return -ENOMEM;
if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
- kfree(pr);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_free_pr;
}
pr->handle = device->handle;
@@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
dev = get_cpu_device(pr->id);
if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
result = -EFAULT;
- goto err_free_cpumask;
+ goto err_clear_processor;
}
/*
@@ -595,9 +595,14 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
err_remove_sysfs:
sysfs_remove_link(&device->dev.kobj, "sysdev");
+err_clear_processor:
+ /* processor_device_array is not cleared to allow checks for buggy
+ BIOSes */
+ per_cpu(processors, pr->id) = NULL;
err_free_cpumask:
free_cpumask_var(pr->throttling.shared_cpu_map);
-
+err_free_pr:
+ kfree(pr);
return result;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-15 8:32 [PATCH] drivers/acpi/processor_driver.c: add missing kfree Julia Lawall
@ 2012-03-15 9:07 ` Srivatsa S. Bhat
2012-03-15 9:32 ` Deepthi Dharwar
1 sibling, 0 replies; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-03-15 9:07 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, deepthi, venki, trenn, bhelgaas, kernel-janitors,
linux-acpi, linux-kernel
On 03/15/2012 02:02 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> The function acpi_processor_add is stored in the ops.add field of a
> acpi_driver structure. This function is then called in
> acpi_bus_driver_init. On failure, this function clears the field
> device->driver_data, but does not free its contents. Thus the free has to
> be done by the add function. In acpi_processor_add, the corresponding
> value is pr. This value is currently freed on failure before storing it in
> device->driver_data, but not after. This free is added in the error
> handling code at the end of the function. The per_cpu variable
> processors is also cleared so that it does not refer to a dangling pointer.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
See a minor nitpick below.. Other than that, everything looks great!
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> drivers/acpi/processor_driver.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 2801b41..98c3648 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> return -ENOMEM;
>
> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
> - kfree(pr);
> - return -ENOMEM;
> + result = -ENOMEM;
> + goto err_free_pr;
> }
>
> pr->handle = device->handle;
> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> dev = get_cpu_device(pr->id);
> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
> result = -EFAULT;
> - goto err_free_cpumask;
> + goto err_clear_processor;
> }
>
> /*
> @@ -595,9 +595,14 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>
> err_remove_sysfs:
> sysfs_remove_link(&device->dev.kobj, "sysdev");
> +err_clear_processor:
> + /* processor_device_array is not cleared to allow checks for buggy
> + BIOSes */
Multi-line comments in the kernel are generally written like:
/*
* blah blah blah
* blah blah ....
*/
> + per_cpu(processors, pr->id) = NULL;
> err_free_cpumask:
> free_cpumask_var(pr->throttling.shared_cpu_map);
> -
> +err_free_pr:
> + kfree(pr);
> return result;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-15 8:32 [PATCH] drivers/acpi/processor_driver.c: add missing kfree Julia Lawall
2012-03-15 9:07 ` Srivatsa S. Bhat
@ 2012-03-15 9:32 ` Deepthi Dharwar
1 sibling, 0 replies; 9+ messages in thread
From: Deepthi Dharwar @ 2012-03-15 9:32 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, srivatsa.bhat, venki, trenn, bhelgaas, kernel-janitors,
linux-acpi, linux-kernel
On 03/15/2012 02:02 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> The function acpi_processor_add is stored in the ops.add field of a
> acpi_driver structure. This function is then called in
> acpi_bus_driver_init. On failure, this function clears the field
> device->driver_data, but does not free its contents. Thus the free has to
> be done by the add function. In acpi_processor_add, the corresponding
> value is pr. This value is currently freed on failure before storing it in
> device->driver_data, but not after. This free is added in the error
> handling code at the end of the function. The per_cpu variable
> processors is also cleared so that it does not refer to a dangling pointer.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
Acked-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> ---
> drivers/acpi/processor_driver.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 2801b41..98c3648 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> return -ENOMEM;
>
> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
> - kfree(pr);
> - return -ENOMEM;
> + result = -ENOMEM;
> + goto err_free_pr;
> }
>
> pr->handle = device->handle;
> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> dev = get_cpu_device(pr->id);
> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
> result = -EFAULT;
> - goto err_free_cpumask;
> + goto err_clear_processor;
> }
>
> /*
> @@ -595,9 +595,14 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>
> err_remove_sysfs:
> sysfs_remove_link(&device->dev.kobj, "sysdev");
> +err_clear_processor:
> + /* processor_device_array is not cleared to allow checks for buggy
> + BIOSes */
> + per_cpu(processors, pr->id) = NULL;
> err_free_cpumask:
> free_cpumask_var(pr->throttling.shared_cpu_map);
> -
> +err_free_pr:
> + kfree(pr);
> return result;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drivers/acpi/processor_driver.c: add missing kfree
@ 2012-03-16 10:02 Julia Lawall
0 siblings, 0 replies; 9+ messages in thread
From: Julia Lawall @ 2012-03-16 10:02 UTC (permalink / raw)
To: Len Brown, srivatsa.bhat, deepthi, venki, trenn, bhelgaas
Cc: kernel-janitors, linux-acpi, linux-kernel
From: Julia Lawall <Julia.Lawall@lip6.fr>
The function acpi_processor_add is stored in the ops.add field of a
acpi_driver structure. This function is then called in
acpi_bus_driver_init. On failure, this function clears the field
device->driver_data, but does not free its contents. Thus the free has to
be done by the add function. In acpi_processor_add, the corresponding
value is pr. This value is currently freed on failure before storing it in
device->driver_data, but not after. This free is added in the error
handling code at the end of the function. The per_cpu variable
processors is also cleared so that it does not refer to a dangling pointer.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
v2: Adjusted the comment format
drivers/acpi/processor_driver.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 2801b41..c5ce91c 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
return -ENOMEM;
if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
- kfree(pr);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_free_pr;
}
pr->handle = device->handle;
@@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
dev = get_cpu_device(pr->id);
if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
result = -EFAULT;
- goto err_free_cpumask;
+ goto err_clear_processor;
}
/*
@@ -595,9 +595,16 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
err_remove_sysfs:
sysfs_remove_link(&device->dev.kobj, "sysdev");
+err_clear_processor:
+ /*
+ * processor_device_array is not cleared to allow checks for buggy
+ * BIOSes
+ */
+ per_cpu(processors, pr->id) = NULL;
err_free_cpumask:
free_cpumask_var(pr->throttling.shared_cpu_map);
-
+err_free_pr:
+ kfree(pr);
return result;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] drivers/acpi/processor_driver.c: add missing kfree
@ 2012-03-17 15:38 Julia Lawall
0 siblings, 0 replies; 9+ messages in thread
From: Julia Lawall @ 2012-03-17 15:38 UTC (permalink / raw)
To: Len Brown, srivatsa.bhat, deepthi, venki, trenn, bhelgaas
Cc: kernel-janitors, linux-acpi, linux-kernel
From: Julia Lawall <Julia.Lawall@lip6.fr>
The function acpi_processor_add is stored in the ops.add field of a
acpi_driver structure. This function is then called in
acpi_bus_driver_init. On failure, this function clears the field
device->driver_data, but does not free its contents. Thus the free has to
be done by the add function. In acpi_processor_add, the corresponding
value is pr. This value is currently freed on failure before storing it in
device->driver_data, but not after. This free is added in the error
handling code at the end of the function. The per_cpu variable
processors is also cleared so that it does not refer to a dangling pointer.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Acked-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
---
v3: Added reviews and acks
drivers/acpi/processor_driver.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 2801b41..c5ce91c 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
return -ENOMEM;
if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
- kfree(pr);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_free_pr;
}
pr->handle = device->handle;
@@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
dev = get_cpu_device(pr->id);
if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
result = -EFAULT;
- goto err_free_cpumask;
+ goto err_clear_processor;
}
/*
@@ -595,9 +595,16 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
err_remove_sysfs:
sysfs_remove_link(&device->dev.kobj, "sysdev");
+err_clear_processor:
+ /*
+ * processor_device_array is not cleared to allow checks for buggy
+ * BIOSes
+ */
+ per_cpu(processors, pr->id) = NULL;
err_free_cpumask:
free_cpumask_var(pr->throttling.shared_cpu_map);
-
+err_free_pr:
+ kfree(pr);
return result;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-03-17 15:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-15 8:32 [PATCH] drivers/acpi/processor_driver.c: add missing kfree Julia Lawall
2012-03-15 9:07 ` Srivatsa S. Bhat
2012-03-15 9:32 ` Deepthi Dharwar
-- strict thread matches above, loose matches on Subject: below --
2012-03-17 15:38 Julia Lawall
2012-03-16 10:02 Julia Lawall
2012-03-10 17:05 Julia Lawall
2012-03-14 11:34 ` Srivatsa S. Bhat
2012-03-14 12:32 ` Julia Lawall
2012-03-14 12:50 ` Srivatsa S. Bhat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox