[PATCH v2] module: check if memory leak by module.

[PATCH v2] module: check if memory leak by module.

[Maninder Singh]

This patch checks if any module which is going to be unloaded
is doing vmalloc memory leak or not.

Logs:-
[  129.336368] Module [test_module] is getting unloaded before doing vfree
[  129.336371] Memory still allocated: addr:0xffffc90001461000 - 0xffffc900014c7000, pages 101
[  129.336376] Allocating function kernel_init+0x1c/0x20 [test_module]

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
v1->v2: made code generic rather than dependent on config.
	changed pr_alert to pr_err.

 include/linux/vmalloc.h |  2 ++
 kernel/module.c         | 22 ++++++++++++++++++++++
 mm/vmalloc.c            |  2 --
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 46991ad..5531af3 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -29,6 +29,8 @@
 #define IOREMAP_MAX_ORDER	(7 + PAGE_SHIFT)	/* 128 pages */
 #endif
 
+#define VM_VM_AREA  0x04
+
 struct vm_struct {
 	struct vm_struct	*next;
 	void			*addr;
diff --git a/kernel/module.c b/kernel/module.c
index f953df9..98a8018 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2117,9 +2117,31 @@ void __weak module_arch_freeing_init(struct module *mod)
 {
 }
 
+static void check_memory_leak(struct module *mod)
+{
+	struct vmap_area *va;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(va, &vmap_area_list, list) {
+		if (!(va->flags & VM_VM_AREA))
+			continue;
+		if ((mod->core_layout.base < va->vm->caller) &&
+			(mod->core_layout.base + mod->core_layout.size) > va->vm->caller) {
+			pr_err("Module [%s] is getting unloaded before doing vfree\n", mod->name);
+			pr_err("Memory still allocated: addr:0x%lx - 0x%lx, pages %u\n",
+				va->va_start, va->va_end, va->vm->nr_pages);
+			pr_err("Allocating function %pS\n", va->vm->caller);
+		}
+
+	}
+	rcu_read_unlock();
+}
+
 /* Free a module, remove from lists, etc. */
 static void free_module(struct module *mod)
 {
+	check_memory_leak(mod);
+
 	trace_module_free(mod);
 
 	mod_sysfs_teardown(mod);
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 68eb002..0166a0a 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -314,8 +314,6 @@ unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
 
 /*** Global kva allocator ***/
 
-#define VM_VM_AREA	0x04
-
 static DEFINE_SPINLOCK(vmap_area_lock);
 /* Export for kexec only */
 LIST_HEAD(vmap_area_list);
-- 
1.9.1

[PATCH v2] module: check if memory leak by module.

[Maninder Singh]

This patch checks if any module which is going to be unloaded
is doing vmalloc memory leak or not.

Logs:-
[  129.336368] Module [test_module] is getting unloaded before doing vfree
[  129.336371] Memory still allocated: addr:0xffffc90001461000 - 0xffffc900014c7000, pages 101
[  129.336376] Allocating function kernel_init+0x1c/0x20 [test_module]

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
v1->v2: made code generic rather than dependent on config.
	changed pr_alert to pr_err.

 include/linux/vmalloc.h |  2 ++
 kernel/module.c         | 22 ++++++++++++++++++++++
 mm/vmalloc.c            |  2 --
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 46991ad..5531af3 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -29,6 +29,8 @@
 #define IOREMAP_MAX_ORDER	(7 + PAGE_SHIFT)	/* 128 pages */
 #endif
 
+#define VM_VM_AREA  0x04
+
 struct vm_struct {
 	struct vm_struct	*next;
 	void			*addr;
diff --git a/kernel/module.c b/kernel/module.c
index f953df9..98a8018 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2117,9 +2117,31 @@ void __weak module_arch_freeing_init(struct module *mod)
 {
 }
 
+static void check_memory_leak(struct module *mod)
+{
+	struct vmap_area *va;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(va, &vmap_area_list, list) {
+		if (!(va->flags & VM_VM_AREA))
+			continue;
+		if ((mod->core_layout.base < va->vm->caller) &&
+			(mod->core_layout.base + mod->core_layout.size) > va->vm->caller) {
+			pr_err("Module [%s] is getting unloaded before doing vfree\n", mod->name);
+			pr_err("Memory still allocated: addr:0x%lx - 0x%lx, pages %u\n",
+				va->va_start, va->va_end, va->vm->nr_pages);
+			pr_err("Allocating function %pS\n", va->vm->caller);
+		}
+
+	}
+	rcu_read_unlock();
+}
+
 /* Free a module, remove from lists, etc. */
 static void free_module(struct module *mod)
 {
+	check_memory_leak(mod);
+
 	trace_module_free(mod);
 
 	mod_sysfs_teardown(mod);
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 68eb002..0166a0a 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -314,8 +314,6 @@ unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
 
 /*** Global kva allocator ***/
 
-#define VM_VM_AREA	0x04
-
 static DEFINE_SPINLOCK(vmap_area_lock);
 /* Export for kexec only */
 LIST_HEAD(vmap_area_list);
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Wed 29-03-17 11:32:02, Maninder Singh wrote:
> This patch checks if any module which is going to be unloaded
> is doing vmalloc memory leak or not.

Hmm, how can you track _all_ vmalloc allocations done on behalf of the
module? It is quite some time since I've checked kernel/module.c but
from my vague understading your check is basically only about statically
vmalloced areas by module loader. Is that correct? If yes then is this
actually useful? Were there any bugs in the loader code recently? What
led you to prepare this patch? All this should be part of the changelog!
 
> Logs:-
> [  129.336368] Module [test_module] is getting unloaded before doing vfree
> [  129.336371] Memory still allocated: addr:0xffffc90001461000 - 0xffffc900014c7000, pages 101
> [  129.336376] Allocating function kernel_init+0x1c/0x20 [test_module]
> 
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
> v1->v2: made code generic rather than dependent on config.
> 	changed pr_alert to pr_err.
> 
>  include/linux/vmalloc.h |  2 ++
>  kernel/module.c         | 22 ++++++++++++++++++++++
>  mm/vmalloc.c            |  2 --
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 46991ad..5531af3 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -29,6 +29,8 @@
>  #define IOREMAP_MAX_ORDER	(7 + PAGE_SHIFT)	/* 128 pages */
>  #endif
>  
> +#define VM_VM_AREA  0x04
> +
>  struct vm_struct {
>  	struct vm_struct	*next;
>  	void			*addr;
> diff --git a/kernel/module.c b/kernel/module.c
> index f953df9..98a8018 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2117,9 +2117,31 @@ void __weak module_arch_freeing_init(struct module *mod)
>  {
>  }
>  
> +static void check_memory_leak(struct module *mod)
> +{
> +	struct vmap_area *va;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(va, &vmap_area_list, list) {
> +		if (!(va->flags & VM_VM_AREA))
> +			continue;
> +		if ((mod->core_layout.base < va->vm->caller) &&
> +			(mod->core_layout.base + mod->core_layout.size) > va->vm->caller) {
> +			pr_err("Module [%s] is getting unloaded before doing vfree\n", mod->name);
> +			pr_err("Memory still allocated: addr:0x%lx - 0x%lx, pages %u\n",
> +				va->va_start, va->va_end, va->vm->nr_pages);
> +			pr_err("Allocating function %pS\n", va->vm->caller);
> +		}
> +
> +	}
> +	rcu_read_unlock();
> +}
> +
>  /* Free a module, remove from lists, etc. */
>  static void free_module(struct module *mod)
>  {
> +	check_memory_leak(mod);
> +
>  	trace_module_free(mod);
>  
>  	mod_sysfs_teardown(mod);
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 68eb002..0166a0a 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -314,8 +314,6 @@ unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
>  
>  /*** Global kva allocator ***/
>  
> -#define VM_VM_AREA	0x04
> -
>  static DEFINE_SPINLOCK(vmap_area_lock);
>  /* Export for kexec only */
>  LIST_HEAD(vmap_area_list);
> -- 
> 1.9.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Wed 29-03-17 11:32:02, Maninder Singh wrote:
> This patch checks if any module which is going to be unloaded
> is doing vmalloc memory leak or not.

Hmm, how can you track _all_ vmalloc allocations done on behalf of the
module? It is quite some time since I've checked kernel/module.c but
from my vague understading your check is basically only about statically
vmalloced areas by module loader. Is that correct? If yes then is this
actually useful? Were there any bugs in the loader code recently? What
led you to prepare this patch? All this should be part of the changelog!
 
> Logs:-
> [  129.336368] Module [test_module] is getting unloaded before doing vfree
> [  129.336371] Memory still allocated: addr:0xffffc90001461000 - 0xffffc900014c7000, pages 101
> [  129.336376] Allocating function kernel_init+0x1c/0x20 [test_module]
> 
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
> v1->v2: made code generic rather than dependent on config.
> 	changed pr_alert to pr_err.
> 
>  include/linux/vmalloc.h |  2 ++
>  kernel/module.c         | 22 ++++++++++++++++++++++
>  mm/vmalloc.c            |  2 --
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 46991ad..5531af3 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -29,6 +29,8 @@
>  #define IOREMAP_MAX_ORDER	(7 + PAGE_SHIFT)	/* 128 pages */
>  #endif
>  
> +#define VM_VM_AREA  0x04
> +
>  struct vm_struct {
>  	struct vm_struct	*next;
>  	void			*addr;
> diff --git a/kernel/module.c b/kernel/module.c
> index f953df9..98a8018 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2117,9 +2117,31 @@ void __weak module_arch_freeing_init(struct module *mod)
>  {
>  }
>  
> +static void check_memory_leak(struct module *mod)
> +{
> +	struct vmap_area *va;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(va, &vmap_area_list, list) {
> +		if (!(va->flags & VM_VM_AREA))
> +			continue;
> +		if ((mod->core_layout.base < va->vm->caller) &&
> +			(mod->core_layout.base + mod->core_layout.size) > va->vm->caller) {
> +			pr_err("Module [%s] is getting unloaded before doing vfree\n", mod->name);
> +			pr_err("Memory still allocated: addr:0x%lx - 0x%lx, pages %u\n",
> +				va->va_start, va->va_end, va->vm->nr_pages);
> +			pr_err("Allocating function %pS\n", va->vm->caller);
> +		}
> +
> +	}
> +	rcu_read_unlock();
> +}
> +
>  /* Free a module, remove from lists, etc. */
>  static void free_module(struct module *mod)
>  {
> +	check_memory_leak(mod);
> +
>  	trace_module_free(mod);
>  
>  	mod_sysfs_teardown(mod);
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 68eb002..0166a0a 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -314,8 +314,6 @@ unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
>  
>  /*** Global kva allocator ***/
>  
> -#define VM_VM_AREA	0x04
> -
>  static DEFINE_SPINLOCK(vmap_area_lock);
>  /* Export for kexec only */
>  LIST_HEAD(vmap_area_list);
> -- 
> 1.9.1
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Miroslav Benes]

On Wed, 29 Mar 2017, Michal Hocko wrote:

> On Wed 29-03-17 11:32:02, Maninder Singh wrote:
> > This patch checks if any module which is going to be unloaded
> > is doing vmalloc memory leak or not.
> 
> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> module? It is quite some time since I've checked kernel/module.c but
> from my vague understading your check is basically only about statically
> vmalloced areas by module loader. Is that correct? If yes then is this
> actually useful? Were there any bugs in the loader code recently? What
> led you to prepare this patch? All this should be part of the changelog!

Moreover, I don't understand one thing:
  
> > Logs:-
> > [  129.336368] Module [test_module] is getting unloaded before doing vfree

ok, but...

> > +static void check_memory_leak(struct module *mod)
> > +{
> > +	struct vmap_area *va;
> > +
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(va, &vmap_area_list, list) {
> > +		if (!(va->flags & VM_VM_AREA))
> > +			continue;
> > +		if ((mod->core_layout.base < va->vm->caller) &&
> > +			(mod->core_layout.base + mod->core_layout.size) > va->vm->caller) {
> > +			pr_err("Module [%s] is getting unloaded before doing vfree\n", mod->name);
> > +			pr_err("Memory still allocated: addr:0x%lx - 0x%lx, pages %u\n",
> > +				va->va_start, va->va_end, va->vm->nr_pages);
> > +			pr_err("Allocating function %pS\n", va->vm->caller);
> > +		}
> > +
> > +	}
> > +	rcu_read_unlock();
> > +}
> > +
> >  /* Free a module, remove from lists, etc. */
> >  static void free_module(struct module *mod)
> >  {
> > +	check_memory_leak(mod);
> > +

Of course, vfree() has not been called yet. It is the beginning of 
free_module(). vfree() is one of the last things you need to do. See 
module_memfree(). If I am not missing something, you get pr_err() 
everytime a module is unloaded.

Regards,
Miroslav

Re: [PATCH v2] module: check if memory leak by module.

[Miroslav Benes]

On Wed, 29 Mar 2017, Michal Hocko wrote:

> On Wed 29-03-17 11:32:02, Maninder Singh wrote:
> > This patch checks if any module which is going to be unloaded
> > is doing vmalloc memory leak or not.
> 
> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> module? It is quite some time since I've checked kernel/module.c but
> from my vague understading your check is basically only about statically
> vmalloced areas by module loader. Is that correct? If yes then is this
> actually useful? Were there any bugs in the loader code recently? What
> led you to prepare this patch? All this should be part of the changelog!

Moreover, I don't understand one thing:
  
> > Logs:-
> > [  129.336368] Module [test_module] is getting unloaded before doing vfree

ok, but...

> > +static void check_memory_leak(struct module *mod)
> > +{
> > +	struct vmap_area *va;
> > +
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(va, &vmap_area_list, list) {
> > +		if (!(va->flags & VM_VM_AREA))
> > +			continue;
> > +		if ((mod->core_layout.base < va->vm->caller) &&
> > +			(mod->core_layout.base + mod->core_layout.size) > va->vm->caller) {
> > +			pr_err("Module [%s] is getting unloaded before doing vfree\n", mod->name);
> > +			pr_err("Memory still allocated: addr:0x%lx - 0x%lx, pages %u\n",
> > +				va->va_start, va->va_end, va->vm->nr_pages);
> > +			pr_err("Allocating function %pS\n", va->vm->caller);
> > +		}
> > +
> > +	}
> > +	rcu_read_unlock();
> > +}
> > +
> >  /* Free a module, remove from lists, etc. */
> >  static void free_module(struct module *mod)
> >  {
> > +	check_memory_leak(mod);
> > +

Of course, vfree() has not been called yet. It is the beginning of 
free_module(). vfree() is one of the last things you need to do. See 
module_memfree(). If I am not missing something, you get pr_err() 
everytime a module is unloaded.

Regards,
Miroslav

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Vaneet Narang]

[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]

Hi,

>> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
>> module? It is quite some time since I've checked kernel/module.c but
>> from my vague understading your check is basically only about statically
>> vmalloced areas by module loader. Is that correct? If yes then is this
>> actually useful? Were there any bugs in the loader code recently? What
>> led you to prepare this patch? All this should be part of the changelog!

First of all there is no issue in kernel/module.c. This patch add functionality
to detect scenario where some kernel module does some memory allocation but gets
unloaded without doing vfree. For example
static int kernel_init(void)
{
        char * ptr = vmalloc(400 * 1024);
        return 0;
}

static void kernel_exit(void)
{        
}

Now in this case if we do rmmod then memory allocated by kernel_init
will not be freed but this patch will detect such kind of bugs in kernel module 
code.

Also We have seen bugs in some kernel modules where they allocate some memory and
gets removed without freeing them and if new module gets loaded in place
of removed module then /proc/vmallocinfo shows wrong information. vmalloc info will
show pages getting allocated by new module. So these logs will help in detecting 
such issues.

> >  static void free_module(struct module *mod)
> >  {
> > +	check_memory_leak(mod);
> > +

>Of course, vfree() has not been called yet. It is the beginning of 
>free_module(). vfree() is one of the last things you need to do. See 
>module_memfree(). If I am not missing something, you get pr_err() 
>everytime a module is unloaded.

This patch is not to detect memory allocated by kernel. module_memfree
will allocated by kernel for kernel modules but our intent is to detect
memory allocated directly by kernel modules and not getting freed.

Regards,
Vaneet Narang

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
> Hi,
> 
> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> >> module? It is quite some time since I've checked kernel/module.c but
> >> from my vague understading your check is basically only about statically
> >> vmalloced areas by module loader. Is that correct? If yes then is this
> >> actually useful? Were there any bugs in the loader code recently? What
> >> led you to prepare this patch? All this should be part of the changelog!
> 
> First of all there is no issue in kernel/module.c. This patch add functionality
> to detect scenario where some kernel module does some memory allocation but gets
> unloaded without doing vfree. For example
> static int kernel_init(void)
> {
>         char * ptr = vmalloc(400 * 1024);
>         return 0;
> }

How can you track that allocation back to the module? Does this patch
actually works at all? Also why would be vmalloc more important than
kmalloc allocations?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
> Hi,
> 
> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> >> module? It is quite some time since I've checked kernel/module.c but
> >> from my vague understading your check is basically only about statically
> >> vmalloced areas by module loader. Is that correct? If yes then is this
> >> actually useful? Were there any bugs in the loader code recently? What
> >> led you to prepare this patch? All this should be part of the changelog!
> 
> First of all there is no issue in kernel/module.c. This patch add functionality
> to detect scenario where some kernel module does some memory allocation but gets
> unloaded without doing vfree. For example
> static int kernel_init(void)
> {
>         char * ptr = vmalloc(400 * 1024);
>         return 0;
> }

How can you track that allocation back to the module? Does this patch
actually works at all? Also why would be vmalloc more important than
kmalloc allocations?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Andrey Ryabinin]

On 03/29/2017 09:02 AM, Maninder Singh wrote:

> diff --git a/kernel/module.c b/kernel/module.c
> index f953df9..98a8018 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2117,9 +2117,31 @@ void __weak module_arch_freeing_init(struct module *mod)
>  {
>  }
>  
> +static void check_memory_leak(struct module *mod)
> +{
> +	struct vmap_area *va;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(va, &vmap_area_list, list) {

vmap_area_list is protected by spin_lock(&vmap_area_lock); not the RCU.

Also some other points:
 1) kmemleak already detects leaks of that kind.

 2) All this could be implemented in userspace, e.g. in rmmod tool
      - Read /proc/vmalloc and find areas belonging to the module
      - unload module
      - read /proc/vmalloc and check if anything left from that module

 3) This might produce false positives. E.g. module may defer vfree() in workqueue, so the 
     actual vfree() call happens after module unloaded.

Re: [PATCH v2] module: check if memory leak by module.

[Andrey Ryabinin]

On 03/29/2017 09:02 AM, Maninder Singh wrote:

> diff --git a/kernel/module.c b/kernel/module.c
> index f953df9..98a8018 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2117,9 +2117,31 @@ void __weak module_arch_freeing_init(struct module *mod)
>  {
>  }
>  
> +static void check_memory_leak(struct module *mod)
> +{
> +	struct vmap_area *va;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(va, &vmap_area_list, list) {

vmap_area_list is protected by spin_lock(&vmap_area_lock); not the RCU.

Also some other points:
 1) kmemleak already detects leaks of that kind.

 2) All this could be implemented in userspace, e.g. in rmmod tool
      - Read /proc/vmalloc and find areas belonging to the module
      - unload module
      - read /proc/vmalloc and check if anything left from that module

 3) This might produce false positives. E.g. module may defer vfree() in workqueue, so the 
     actual vfree() call happens after module unloaded.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

 
>  3) This might produce false positives. E.g. module may defer vfree() in workqueue, so the 
>      actual vfree() call happens after module unloaded.

Umm. Really?

I agree that module may alloc memory and pass it to someone else. Ok
so far.

But if module code executes after module is unloaded -- that is use
after free -- right?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2] module: check if memory leak by module.

[Andrey Ryabinin]

On 03/30/2017 04:37 PM, Pavel Machek wrote:
>  
>>  3) This might produce false positives. E.g. module may defer vfree() in workqueue, so the 
>>      actual vfree() call happens after module unloaded.
> 
> Umm. Really?
> 

I should have been more specific. I meant vfree() called by module from the interrupt context.
In that case the actual __vunmap() will be deferred via schedule_work() thus it might happen
after the module unloaded.
See 32fcfd40715e ("make vfree() safe to call from interrupt contexts")

> I agree that module may alloc memory and pass it to someone else. Ok
> so far.
> 

Right. In the case with vfree() from interrupt we actually pass the memory to
the core code to free it later. 

> But if module code executes after module is unloaded -- that is use
> after free -- right?

Sure, module code can't execute after module unloaded, it doesn't exist anymore.

Re: [PATCH v2] module: check if memory leak by module.

[Andrey Ryabinin]

On 03/30/2017 04:37 PM, Pavel Machek wrote:
>  
>>  3) This might produce false positives. E.g. module may defer vfree() in workqueue, so the 
>>      actual vfree() call happens after module unloaded.
> 
> Umm. Really?
> 

I should have been more specific. I meant vfree() called by module from the interrupt context.
In that case the actual __vunmap() will be deferred via schedule_work() thus it might happen
after the module unloaded.
See 32fcfd40715e ("make vfree() safe to call from interrupt contexts")

> I agree that module may alloc memory and pass it to someone else. Ok
> so far.
> 

Right. In the case with vfree() from interrupt we actually pass the memory to
the core code to free it later. 

> But if module code executes after module is unloaded -- that is use
> after free -- right?

Sure, module code can't execute after module unloaded, it doesn't exist anymore.




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Joel Fernandes]

Hi Michal,

On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
>> Hi,
>>
>> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
>> >> module? It is quite some time since I've checked kernel/module.c but
>> >> from my vague understading your check is basically only about statically
>> >> vmalloced areas by module loader. Is that correct? If yes then is this
>> >> actually useful? Were there any bugs in the loader code recently? What
>> >> led you to prepare this patch? All this should be part of the changelog!
>>
>> First of all there is no issue in kernel/module.c. This patch add functionality
>> to detect scenario where some kernel module does some memory allocation but gets
>> unloaded without doing vfree. For example
>> static int kernel_init(void)
>> {
>>         char * ptr = vmalloc(400 * 1024);
>>         return 0;
>> }
>
> How can you track that allocation back to the module? Does this patch
> actually works at all? Also why would be vmalloc more important than
> kmalloc allocations?

Doesn't the patch use caller's (in this case, the module is the
caller) text address for tracking this? vma->vm->caller should track
the caller doing the allocation?

>From the code:
vmalloc -> __vmalloc_node_flags

In __vmalloc_node_flags:
        return __vmalloc_node(size, 1, flags, PAGE_KERNEL,
                                        node, __builtin_return_address(0));

Since __vmalloc_node_flags is marked as inline, I believe the
__builtin_return_address(0) will return the return address of the
original vmalloc() call which is in the module calling vmalloc.

Regards,
Joel

Re: [PATCH v2] module: check if memory leak by module.

[Joel Fernandes]

Hi Michal,

On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
>> Hi,
>>
>> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
>> >> module? It is quite some time since I've checked kernel/module.c but
>> >> from my vague understading your check is basically only about statically
>> >> vmalloced areas by module loader. Is that correct? If yes then is this
>> >> actually useful? Were there any bugs in the loader code recently? What
>> >> led you to prepare this patch? All this should be part of the changelog!
>>
>> First of all there is no issue in kernel/module.c. This patch add functionality
>> to detect scenario where some kernel module does some memory allocation but gets
>> unloaded without doing vfree. For example
>> static int kernel_init(void)
>> {
>>         char * ptr = vmalloc(400 * 1024);
>>         return 0;
>> }
>
> How can you track that allocation back to the module? Does this patch
> actually works at all? Also why would be vmalloc more important than
> kmalloc allocations?

Doesn't the patch use caller's (in this case, the module is the
caller) text address for tracking this? vma->vm->caller should track
the caller doing the allocation?

>From the code:
vmalloc -> __vmalloc_node_flags

In __vmalloc_node_flags:
        return __vmalloc_node(size, 1, flags, PAGE_KERNEL,
                                        node, __builtin_return_address(0));

Since __vmalloc_node_flags is marked as inline, I believe the
__builtin_return_address(0) will return the return address of the
original vmalloc() call which is in the module calling vmalloc.

Regards,
Joel

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Thu 30-03-17 23:49:52, Joel Fernandes wrote:
> Hi Michal,
> 
> On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
> >> Hi,
> >>
> >> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> >> >> module? It is quite some time since I've checked kernel/module.c but
> >> >> from my vague understading your check is basically only about statically
> >> >> vmalloced areas by module loader. Is that correct? If yes then is this
> >> >> actually useful? Were there any bugs in the loader code recently? What
> >> >> led you to prepare this patch? All this should be part of the changelog!
> >>
> >> First of all there is no issue in kernel/module.c. This patch add functionality
> >> to detect scenario where some kernel module does some memory allocation but gets
> >> unloaded without doing vfree. For example
> >> static int kernel_init(void)
> >> {
> >>         char * ptr = vmalloc(400 * 1024);
> >>         return 0;
> >> }
> >
> > How can you track that allocation back to the module? Does this patch
> > actually works at all? Also why would be vmalloc more important than
> > kmalloc allocations?
> 
> Doesn't the patch use caller's (in this case, the module is the
> caller) text address for tracking this? vma->vm->caller should track
> the caller doing the allocation?

Not really. First of all it will be vmalloc() to be tracked in the above
the example because vmalloc is not inlined. And secondly even if the
caller of the vmalloc was tracked then it would be hopelessly
insufficient because you would get coverage of the _direct_ module usage
of vmalloc rather than anything that the module triggered and that is
outside of the module. Which means any library function etc...
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Thu 30-03-17 23:49:52, Joel Fernandes wrote:
> Hi Michal,
> 
> On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
> >> Hi,
> >>
> >> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> >> >> module? It is quite some time since I've checked kernel/module.c but
> >> >> from my vague understading your check is basically only about statically
> >> >> vmalloced areas by module loader. Is that correct? If yes then is this
> >> >> actually useful? Were there any bugs in the loader code recently? What
> >> >> led you to prepare this patch? All this should be part of the changelog!
> >>
> >> First of all there is no issue in kernel/module.c. This patch add functionality
> >> to detect scenario where some kernel module does some memory allocation but gets
> >> unloaded without doing vfree. For example
> >> static int kernel_init(void)
> >> {
> >>         char * ptr = vmalloc(400 * 1024);
> >>         return 0;
> >> }
> >
> > How can you track that allocation back to the module? Does this patch
> > actually works at all? Also why would be vmalloc more important than
> > kmalloc allocations?
> 
> Doesn't the patch use caller's (in this case, the module is the
> caller) text address for tracking this? vma->vm->caller should track
> the caller doing the allocation?

Not really. First of all it will be vmalloc() to be tracked in the above
the example because vmalloc is not inlined. And secondly even if the
caller of the vmalloc was tracked then it would be hopelessly
insufficient because you would get coverage of the _direct_ module usage
of vmalloc rather than anything that the module triggered and that is
outside of the module. Which means any library function etc...
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Joel Fernandes]

On Fri, Mar 31, 2017 at 1:00 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 30-03-17 23:49:52, Joel Fernandes wrote:
>> Hi Michal,
>>
>> On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
>> >> Hi,
>> >>
>> >> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
>> >> >> module? It is quite some time since I've checked kernel/module.c but
>> >> >> from my vague understading your check is basically only about statically
>> >> >> vmalloced areas by module loader. Is that correct? If yes then is this
>> >> >> actually useful? Were there any bugs in the loader code recently? What
>> >> >> led you to prepare this patch? All this should be part of the changelog!
>> >>
>> >> First of all there is no issue in kernel/module.c. This patch add functionality
>> >> to detect scenario where some kernel module does some memory allocation but gets
>> >> unloaded without doing vfree. For example
>> >> static int kernel_init(void)
>> >> {
>> >>         char * ptr = vmalloc(400 * 1024);
>> >>         return 0;
>> >> }
>> >
>> > How can you track that allocation back to the module? Does this patch
>> > actually works at all? Also why would be vmalloc more important than
>> > kmalloc allocations?
>>
>> Doesn't the patch use caller's (in this case, the module is the
>> caller) text address for tracking this? vma->vm->caller should track
>> the caller doing the allocation?
>
> Not really. First of all it will be vmalloc() to be tracked in the above
> the example because vmalloc is not inlined. And secondly even if the

vmalloc is not inlined, but __built_in_address(0) will return the
*return address* of vmalloc:

>From https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Return-Address.html :
"The level argument is number of frames to scan up the call stack. A
value of 0 yields the return address of the current function"

> caller of the vmalloc was tracked then it would be hopelessly
> insufficient because you would get coverage of the _direct_ module usage
> of vmalloc rather than anything that the module triggered and that is
> outside of the module. Which means any library function etc...

Yes true, but I think the check is for direct allocations, done by the
module, not indirect ones... it may not be a catch-all issues type of
deal but is still IMO a good check since we already have
va->vm->caller available.

Thanks,
Joel

Re: [PATCH v2] module: check if memory leak by module.

[Joel Fernandes]

On Fri, Mar 31, 2017 at 1:00 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 30-03-17 23:49:52, Joel Fernandes wrote:
>> Hi Michal,
>>
>> On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
>> >> Hi,
>> >>
>> >> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
>> >> >> module? It is quite some time since I've checked kernel/module.c but
>> >> >> from my vague understading your check is basically only about statically
>> >> >> vmalloced areas by module loader. Is that correct? If yes then is this
>> >> >> actually useful? Were there any bugs in the loader code recently? What
>> >> >> led you to prepare this patch? All this should be part of the changelog!
>> >>
>> >> First of all there is no issue in kernel/module.c. This patch add functionality
>> >> to detect scenario where some kernel module does some memory allocation but gets
>> >> unloaded without doing vfree. For example
>> >> static int kernel_init(void)
>> >> {
>> >>         char * ptr = vmalloc(400 * 1024);
>> >>         return 0;
>> >> }
>> >
>> > How can you track that allocation back to the module? Does this patch
>> > actually works at all? Also why would be vmalloc more important than
>> > kmalloc allocations?
>>
>> Doesn't the patch use caller's (in this case, the module is the
>> caller) text address for tracking this? vma->vm->caller should track
>> the caller doing the allocation?
>
> Not really. First of all it will be vmalloc() to be tracked in the above
> the example because vmalloc is not inlined. And secondly even if the

vmalloc is not inlined, but __built_in_address(0) will return the
*return address* of vmalloc:

>From https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Return-Address.html :
"The level argument is number of frames to scan up the call stack. A
value of 0 yields the return address of the current function"

> caller of the vmalloc was tracked then it would be hopelessly
> insufficient because you would get coverage of the _direct_ module usage
> of vmalloc rather than anything that the module triggered and that is
> outside of the module. Which means any library function etc...

Yes true, but I think the check is for direct allocations, done by the
module, not indirect ones... it may not be a catch-all issues type of
deal but is still IMO a good check since we already have
va->vm->caller available.

Thanks,
Joel

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Jessica Yu]

+++ Vaneet Narang [29/03/17 09:23 +0000]:
>Hi,
>
>>> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
>>> module? It is quite some time since I've checked kernel/module.c but
>>> from my vague understading your check is basically only about statically
>>> vmalloced areas by module loader. Is that correct? If yes then is this
>>> actually useful? Were there any bugs in the loader code recently? What
>>> led you to prepare this patch? All this should be part of the changelog!
>
>First of all there is no issue in kernel/module.c. This patch add functionality
>to detect scenario where some kernel module does some memory allocation but gets
>unloaded without doing vfree. For example
>static int kernel_init(void)
>{
>        char * ptr = vmalloc(400 * 1024);
>        return 0;
>}
>
>static void kernel_exit(void)
>{
>}
>
>Now in this case if we do rmmod then memory allocated by kernel_init
>will not be freed but this patch will detect such kind of bugs in kernel module
>code.

kmemleak already detects leaks just like this, and it is not just
limited to vmalloc (but also kmalloc, kmem_cache_alloc, etc). See
mm/kmemleak-test.c, it is exactly like your example.

Also, this patch is currently limited to direct vmalloc allocations
from module core code (since you are only checking for vmalloc callers
originating from mod->core_layout, not mod->init_layout, which is
discarded at the end of do_init_module(). If we want to be complete,
we'd have to do another leak check before module init code is freed.

>Also We have seen bugs in some kernel modules where they allocate some memory and
>gets removed without freeing them and if new module gets loaded in place
>of removed module then /proc/vmallocinfo shows wrong information. vmalloc info will
>show pages getting allocated by new module. So these logs will help in detecting
>such issues.

This is an unfortunate side effect of having dynamically loadable modules.
After a module is gone, sprint_symbol() (which is used to display caller
information in /proc/vmallocinfo) simply cannot trace an address back to
a module that no longer exists, it is a natural limitation, and I'm not really
sure if there's much we can do about it. When chasing leaks like this,
one possibility might be to leave the module loaded so vmallocinfo can report
accurate information, and then compare the reported information after the
module unloads.

And unfortunately, this patch also demonstrates the same problem you're describing:

(1) Load leaky_module and read /proc/vmallocinfo:
0xffffa8570005d000-0xffffa8570005f000    8192 leaky_function+0x2f/0x75 [leaky_module] pages=1 vmalloc N0=1

(2) Unload leaky_module and read /proc/vmallocinfo:
0xffffa8570005d000-0xffffa8570005f000    8192 0xffffffffc038902f pages=1 vmalloc N0=1
                                              ^^^ missing caller symbol since module is now gone
On module unload, your patch prints:
[  289.834428] Module [leaky_module] is getting unloaded before doing vfree
[  289.835226] Memory still allocated: addr:0xffffa8570005d000 - 0xffffa8570005f000, pages 1
[  289.836185] Allocating function leaky_function+0x2f/0x75 [leaky_module]

Ok, so far that looks fine. But kmemleak also provides information about the same leak:

  unreferenced object 0xffffa8570005d000 (size 64):
    comm "insmod", pid 114, jiffies 4294673713 (age 208.968s)
    hex dump (first 32 bytes):
      e6 7e 00 00 00 00 00 00 0a 00 00 00 16 00 00 00  .~..............
      21 52 00 00 00 00 00 00 f4 7e 00 00 00 00 00 00  !R.......~......
    backtrace:
      [<ffffffff838415ca>] kmemleak_alloc+0x4a/0xa0
      [<ffffffff83214df4>] __vmalloc_node_range+0x1e4/0x300
      [<ffffffff83214fb4>] vmalloc+0x54/0x60
      [<ffffffffc038902f>] leaky_function+0x2f/0x75 [leaky_module]
      [<ffffffffc038e00b>] 0xffffffffc038e00b
      [<ffffffff83002193>] do_one_initcall+0x53/0x1a0
      [<ffffffff831bfca1>] do_init_module+0x5f/0x1ff
      [<ffffffff8313189f>] load_module+0x273f/0x2b00
      [<ffffffff83131dc6>] SYSC_init_module+0x166/0x180
      [<ffffffff83131efe>] SyS_init_module+0xe/0x10
      [<ffffffff8384d177>] entry_SYSCALL_64_fastpath+0x1a/0xa9
      [<ffffffffffffffff>] 0xffffffffffffffff

(3) Load test_module, which happens to load where leaky_module used to reside in memory:
0xffffa8570005d000-0xffffa8570005f000    8192 test_module_exit+0x2f/0x1000 [test_module] pages=1 vmalloc N0=1
                                              ^^^ incorrect caller, because test_module loaded where old caller used to be

(4) Unload test_module and your patch prints:
[  459.140089] Module [test_module] is getting unloaded before doing vfree
[  459.140551] Memory still allocated: addr:0xffffa8570005d000 - 0xffffa8570005f000, pages 1
[  459.141127] Allocating function test_module_exit+0x2f/0x1000 [test_module] <- incorrect caller

So unfortunately this patch also runs into the same problem, reporting
the incorrect caller, and I'm not really convinced that this patch
adds new information that isn't already available with kmemleak and
/proc/vmallocinfo.

Jessica

>> >  static void free_module(struct module *mod)
>> >  {
>> > +	check_memory_leak(mod);
>> > +
>
>>Of course, vfree() has not been called yet. It is the beginning of
>>free_module(). vfree() is one of the last things you need to do. See
>>module_memfree(). If I am not missing something, you get pr_err()
>>everytime a module is unloaded.
>
>This patch is not to detect memory allocated by kernel. module_memfree
>will allocated by kernel for kernel modules but our intent is to detect
>memory allocated directly by kernel modules and not getting freed.
>
>Regards,
>Vaneet Narang

Re: [PATCH v2] module: check if memory leak by module.

[Jessica Yu]

+++ Vaneet Narang [29/03/17 09:23 +0000]:
>Hi,
>
>>> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
>>> module? It is quite some time since I've checked kernel/module.c but
>>> from my vague understading your check is basically only about statically
>>> vmalloced areas by module loader. Is that correct? If yes then is this
>>> actually useful? Were there any bugs in the loader code recently? What
>>> led you to prepare this patch? All this should be part of the changelog!
>
>First of all there is no issue in kernel/module.c. This patch add functionality
>to detect scenario where some kernel module does some memory allocation but gets
>unloaded without doing vfree. For example
>static int kernel_init(void)
>{
>        char * ptr = vmalloc(400 * 1024);
>        return 0;
>}
>
>static void kernel_exit(void)
>{
>}
>
>Now in this case if we do rmmod then memory allocated by kernel_init
>will not be freed but this patch will detect such kind of bugs in kernel module
>code.

kmemleak already detects leaks just like this, and it is not just
limited to vmalloc (but also kmalloc, kmem_cache_alloc, etc). See
mm/kmemleak-test.c, it is exactly like your example.

Also, this patch is currently limited to direct vmalloc allocations
from module core code (since you are only checking for vmalloc callers
originating from mod->core_layout, not mod->init_layout, which is
discarded at the end of do_init_module(). If we want to be complete,
we'd have to do another leak check before module init code is freed.

>Also We have seen bugs in some kernel modules where they allocate some memory and
>gets removed without freeing them and if new module gets loaded in place
>of removed module then /proc/vmallocinfo shows wrong information. vmalloc info will
>show pages getting allocated by new module. So these logs will help in detecting
>such issues.

This is an unfortunate side effect of having dynamically loadable modules.
After a module is gone, sprint_symbol() (which is used to display caller
information in /proc/vmallocinfo) simply cannot trace an address back to
a module that no longer exists, it is a natural limitation, and I'm not really
sure if there's much we can do about it. When chasing leaks like this,
one possibility might be to leave the module loaded so vmallocinfo can report
accurate information, and then compare the reported information after the
module unloads.

And unfortunately, this patch also demonstrates the same problem you're describing:

(1) Load leaky_module and read /proc/vmallocinfo:
0xffffa8570005d000-0xffffa8570005f000    8192 leaky_function+0x2f/0x75 [leaky_module] pages=1 vmalloc N0=1

(2) Unload leaky_module and read /proc/vmallocinfo:
0xffffa8570005d000-0xffffa8570005f000    8192 0xffffffffc038902f pages=1 vmalloc N0=1
                                              ^^^ missing caller symbol since module is now gone
On module unload, your patch prints:
[  289.834428] Module [leaky_module] is getting unloaded before doing vfree
[  289.835226] Memory still allocated: addr:0xffffa8570005d000 - 0xffffa8570005f000, pages 1
[  289.836185] Allocating function leaky_function+0x2f/0x75 [leaky_module]

Ok, so far that looks fine. But kmemleak also provides information about the same leak:

  unreferenced object 0xffffa8570005d000 (size 64):
    comm "insmod", pid 114, jiffies 4294673713 (age 208.968s)
    hex dump (first 32 bytes):
      e6 7e 00 00 00 00 00 00 0a 00 00 00 16 00 00 00  .~..............
      21 52 00 00 00 00 00 00 f4 7e 00 00 00 00 00 00  !R.......~......
    backtrace:
      [<ffffffff838415ca>] kmemleak_alloc+0x4a/0xa0
      [<ffffffff83214df4>] __vmalloc_node_range+0x1e4/0x300
      [<ffffffff83214fb4>] vmalloc+0x54/0x60
      [<ffffffffc038902f>] leaky_function+0x2f/0x75 [leaky_module]
      [<ffffffffc038e00b>] 0xffffffffc038e00b
      [<ffffffff83002193>] do_one_initcall+0x53/0x1a0
      [<ffffffff831bfca1>] do_init_module+0x5f/0x1ff
      [<ffffffff8313189f>] load_module+0x273f/0x2b00
      [<ffffffff83131dc6>] SYSC_init_module+0x166/0x180
      [<ffffffff83131efe>] SyS_init_module+0xe/0x10
      [<ffffffff8384d177>] entry_SYSCALL_64_fastpath+0x1a/0xa9
      [<ffffffffffffffff>] 0xffffffffffffffff

(3) Load test_module, which happens to load where leaky_module used to reside in memory:
0xffffa8570005d000-0xffffa8570005f000    8192 test_module_exit+0x2f/0x1000 [test_module] pages=1 vmalloc N0=1
                                              ^^^ incorrect caller, because test_module loaded where old caller used to be

(4) Unload test_module and your patch prints:
[  459.140089] Module [test_module] is getting unloaded before doing vfree
[  459.140551] Memory still allocated: addr:0xffffa8570005d000 - 0xffffa8570005f000, pages 1
[  459.141127] Allocating function test_module_exit+0x2f/0x1000 [test_module] <- incorrect caller

So unfortunately this patch also runs into the same problem, reporting
the incorrect caller, and I'm not really convinced that this patch
adds new information that isn't already available with kmemleak and
/proc/vmallocinfo.

Jessica

>> >  static void free_module(struct module *mod)
>> >  {
>> > +	check_memory_leak(mod);
>> > +
>
>>Of course, vfree() has not been called yet. It is the beginning of
>>free_module(). vfree() is one of the last things you need to do. See
>>module_memfree(). If I am not missing something, you get pr_err()
>>everytime a module is unloaded.
>
>This patch is not to detect memory allocated by kernel. module_memfree
>will allocated by kernel for kernel modules but our intent is to detect
>memory allocated directly by kernel modules and not getting freed.
>
>Regards,
>Vaneet Narang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Fri 31-03-17 10:05:29, Joel Fernandes wrote:
> On Fri, Mar 31, 2017 at 1:00 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Thu 30-03-17 23:49:52, Joel Fernandes wrote:
> >> Hi Michal,
> >>
> >> On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >> > On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
> >> >> Hi,
> >> >>
> >> >> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> >> >> >> module? It is quite some time since I've checked kernel/module.c but
> >> >> >> from my vague understading your check is basically only about statically
> >> >> >> vmalloced areas by module loader. Is that correct? If yes then is this
> >> >> >> actually useful? Were there any bugs in the loader code recently? What
> >> >> >> led you to prepare this patch? All this should be part of the changelog!
> >> >>
> >> >> First of all there is no issue in kernel/module.c. This patch add functionality
> >> >> to detect scenario where some kernel module does some memory allocation but gets
> >> >> unloaded without doing vfree. For example
> >> >> static int kernel_init(void)
> >> >> {
> >> >>         char * ptr = vmalloc(400 * 1024);
> >> >>         return 0;
> >> >> }
> >> >
> >> > How can you track that allocation back to the module? Does this patch
> >> > actually works at all? Also why would be vmalloc more important than
> >> > kmalloc allocations?
> >>
> >> Doesn't the patch use caller's (in this case, the module is the
> >> caller) text address for tracking this? vma->vm->caller should track
> >> the caller doing the allocation?
> >
> > Not really. First of all it will be vmalloc() to be tracked in the above
> > the example because vmalloc is not inlined. And secondly even if the
> 
> vmalloc is not inlined, but __built_in_address(0) will return the
> *return address* of vmalloc:
> 
> >From https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Return-Address.html :
> "The level argument is number of frames to scan up the call stack. A
> value of 0 yields the return address of the current function"

yes, sorry, I meant to say s@vmalloc is not @__vmalloc_node_flags is not@
I can see some arguments to make __vmalloc_node_flags inline to make
/proc/vmallocinfo output more useful but...

> > caller of the vmalloc was tracked then it would be hopelessly
> > insufficient because you would get coverage of the _direct_ module usage
> > of vmalloc rather than anything that the module triggered and that is
> > outside of the module. Which means any library function etc...
> 
> Yes true, but I think the check is for direct allocations, done by the
> module, not indirect ones... it may not be a catch-all issues type of
> deal but is still IMO a good check since we already have
> va->vm->caller available.

I disagree. We have a full featured kmemleak to catch all potential
leaks. This code is IMHO not worth it.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] module: check if memory leak by module.

[Michal Hocko]

On Fri 31-03-17 10:05:29, Joel Fernandes wrote:
> On Fri, Mar 31, 2017 at 1:00 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Thu 30-03-17 23:49:52, Joel Fernandes wrote:
> >> Hi Michal,
> >>
> >> On Wed, Mar 29, 2017 at 3:43 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >> > On Wed 29-03-17 09:23:32, Vaneet Narang wrote:
> >> >> Hi,
> >> >>
> >> >> >> Hmm, how can you track _all_ vmalloc allocations done on behalf of the
> >> >> >> module? It is quite some time since I've checked kernel/module.c but
> >> >> >> from my vague understading your check is basically only about statically
> >> >> >> vmalloced areas by module loader. Is that correct? If yes then is this
> >> >> >> actually useful? Were there any bugs in the loader code recently? What
> >> >> >> led you to prepare this patch? All this should be part of the changelog!
> >> >>
> >> >> First of all there is no issue in kernel/module.c. This patch add functionality
> >> >> to detect scenario where some kernel module does some memory allocation but gets
> >> >> unloaded without doing vfree. For example
> >> >> static int kernel_init(void)
> >> >> {
> >> >>         char * ptr = vmalloc(400 * 1024);
> >> >>         return 0;
> >> >> }
> >> >
> >> > How can you track that allocation back to the module? Does this patch
> >> > actually works at all? Also why would be vmalloc more important than
> >> > kmalloc allocations?
> >>
> >> Doesn't the patch use caller's (in this case, the module is the
> >> caller) text address for tracking this? vma->vm->caller should track
> >> the caller doing the allocation?
> >
> > Not really. First of all it will be vmalloc() to be tracked in the above
> > the example because vmalloc is not inlined. And secondly even if the
> 
> vmalloc is not inlined, but __built_in_address(0) will return the
> *return address* of vmalloc:
> 
> >From https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Return-Address.html :
> "The level argument is number of frames to scan up the call stack. A
> value of 0 yields the return address of the current function"

yes, sorry, I meant to say s@vmalloc is not @__vmalloc_node_flags is not@
I can see some arguments to make __vmalloc_node_flags inline to make
/proc/vmallocinfo output more useful but...

> > caller of the vmalloc was tracked then it would be hopelessly
> > insufficient because you would get coverage of the _direct_ module usage
> > of vmalloc rather than anything that the module triggered and that is
> > outside of the module. Which means any library function etc...
> 
> Yes true, but I think the check is for direct allocations, done by the
> module, not indirect ones... it may not be a catch-all issues type of
> deal but is still IMO a good check since we already have
> va->vm->caller available.

I disagree. We have a full featured kmemleak to catch all potential
leaks. This code is IMHO not worth it.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>