Re: Possible kernel memory leaks

Re: Possible kernel memory leaks

[Catalin Marinas]

Catalin Marinas <catalin.marinas@gmail.com> wrote:
> - acpi_evaluate_integer in drivers/acpi/utils.c - "element" is not freed
> on the error path (if Coverity hasn't seen this, it was probably
> confused by the return_* macros)

This is simpler. I'll send a separate patch for it.

> - acpi_ev_execute_reg_method in drivers/acpi/events/evregion.c - I'm not
> sure about this but kmemleak reports an orphan pointer on the following
> allocation path:
>   c0159372: <kmem_cache_alloc>
>   c01ffa07: <acpi_os_acquire_object>
>   c0215b3a: <acpi_ut_allocate_object_desc_dbg>
>   c02159ce: <acpi_ut_create_internal_object_dbg>
>   c0203784: <acpi_ev_execute_reg_method>
>   c0203db4: <acpi_ev_reg_run>
>   c020ed17: <acpi_ns_walk_namespace>
>   c0203d6b: <acpi_ev_execute_reg_methods>
> Is acpi_ut_remove_reference actually removing the params[0/1]?

I'll need to enable the ACPI debug output as I can't find the leak by
only looking at the code. I'll let you know if there is a leak.

-- 
Catalin

[PATCH] Fix the memory leak in acpi_evaluate_integer()

[Catalin Marinas]

From: Catalin Marinas <catalin.marinas@arm.com>

A leak can happen because of the early returns from this function.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---

 drivers/acpi/utils.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 6458c47..71afcd3 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -273,20 +273,22 @@ acpi_evaluate_integer(acpi_handle handle
 	status = acpi_evaluate_object(handle, pathname, arguments, &buffer);
 	if (ACPI_FAILURE(status)) {
 		acpi_util_eval_error(handle, pathname, status);
-		return_ACPI_STATUS(status);
+		goto out;
 	}
 
 	if (element->type != ACPI_TYPE_INTEGER) {
 		acpi_util_eval_error(handle, pathname, AE_BAD_DATA);
-		return_ACPI_STATUS(AE_BAD_DATA);
+		status = AE_BAD_DATA;
+		goto out;
 	}
 
 	*data = element->integer.value;
+ out:
 	kfree(element);
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Return value [%lu]\n", *data));
 
-	return_ACPI_STATUS(AE_OK);
+	return_ACPI_STATUS(status);
 }
 
 EXPORT_SYMBOL(acpi_evaluate_integer);

Re: Possible kernel memory leaks

[Catalin Marinas]

Catalin Marinas <catalin.marinas@gmail.com> wrote:
> There are some possible kernel memory leaks discovered by kmemleak. I
> didn't have time for investigating them. Please let me know if they are
> not leaks so that I can improve kmemleak (or just add a false alarm call):
[...]
> - acpi_ev_execute_reg_method in drivers/acpi/events/evregion.c - I'm not
> sure about this but kmemleak reports an orphan pointer on the following
> allocation path:
>   c0159372: <kmem_cache_alloc>
>   c01ffa07: <acpi_os_acquire_object>
>   c0215b3a: <acpi_ut_allocate_object_desc_dbg>
>   c02159ce: <acpi_ut_create_internal_object_dbg>
>   c0203784: <acpi_ev_execute_reg_method>
>   c0203db4: <acpi_ev_reg_run>
>   c020ed17: <acpi_ns_walk_namespace>
>   c0203d6b: <acpi_ev_execute_reg_methods>
> Is acpi_ut_remove_reference actually removing the params[0/1]?

After a quick check, the reference counts after the
acpi_ns_evaluate_by_handle() call in acpi_ev_execute_reg_method look
like this (they were both 1 before this call):

  params[0]->common.reference_count = 1
  params[1]->common.reference_count = 2

and therefore acpi_ut_remove_reference() doesn't free
params[1]. Kmemleak, however, cannot find the params[1] value while
scanning the memory and therefore reports it as a leak.

Is this normal behaviour for the acpi_ev_execute_reg_method function?
There isn't anything obvious looking at the calling tree (which I
would say is pretty complex).

-- 
Catalin

Re: Possible kernel memory leaks

[Catalin Marinas]

Catalin Marinas <catalin.marinas@arm.com> wrote:
> Catalin Marinas <catalin.marinas@gmail.com> wrote:
>> There are some possible kernel memory leaks discovered by kmemleak.
> [...]
>> - acpi_ev_execute_reg_method in drivers/acpi/events/evregion.c - I'm not
>> sure about this but kmemleak reports an orphan pointer on the following
>> allocation path:
>>   c0159372: <kmem_cache_alloc>
>>   c01ffa07: <acpi_os_acquire_object>
>>   c0215b3a: <acpi_ut_allocate_object_desc_dbg>
>>   c02159ce: <acpi_ut_create_internal_object_dbg>
>>   c0203784: <acpi_ev_execute_reg_method>
>>   c0203db4: <acpi_ev_reg_run>
>>   c020ed17: <acpi_ns_walk_namespace>
>>   c0203d6b: <acpi_ev_execute_reg_methods>
>> Is acpi_ut_remove_reference actually removing the params[0/1]?
>
> After a quick check, the reference counts after the
> acpi_ns_evaluate_by_handle() call in acpi_ev_execute_reg_method look
> like this (they were both 1 before this call):
>
>   params[0]->common.reference_count = 1
>   params[1]->common.reference_count = 2
>
> and therefore acpi_ut_remove_reference() doesn't free
> params[1]. Kmemleak, however, cannot find the params[1] value while
> scanning the memory and therefore reports it as a leak.

I'll keep investigating this as I think its a real object
leak. Looking at why params[1] has a different reference_count from
params[0], led me to the following backtrace on the ref count
increment (that's getting really complicated):

  acpi_ut_add_reference
  acpi_ds_method_data_get_value
  acpi_ex_resolve_object_to_value
  acpi_ex_resolve_to_value
  acpi_ex_resolve_operands
    (I have a suspicion that the above function should call
     acpi_ut_remove_reference(obj_desc) on an error return path but it
     actually doesn't and, therefore, the ref count remains
     incremented. In this function, params[0] ref count is 3 but the
     one for params[1] becomes 4)
  acpi_ds_exec_end_op (called via walk_state->ascending_callback)
  acpi_ps_parse_loop
  acpi_ps_parse_aml
  acpi_ps_execute_pass
  acpi_ps_execute_method
  acpi_ns_execute_control_method
  acpi_ns_evaluate_by_handle
  acpi_ev_execute_reg_method

Any suggestions/hints? I hope to get to the bottom of this in the next
few days.

Thanks.

-- 
Catalin