Possible kernel memory leaks

Possible kernel memory leaks

[Catalin Marinas]

Hi,

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_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)

- 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]?

And some new false positives (kmemleak needs fixing):

- legacy_init_iomem_resources in arch/i386/kernel/setup.c - kmemleak is
probably right here in that "res" can never be freed since the pointer
was lost. However, there is no need to free this resource and that's why
I'll add a false alarm call

- there are a lot of false positives caused by module loading. I would
need to investigate these a bit more (it's missing a root memory block
that can lead to the reported pointers; it's also missing the static
variables in module)

Thanks,

Catalin

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