[PATCH] module: Refine kmemleak scanned areas

[PATCH] module: Refine kmemleak scanned areas

[Vincent Donnefort]

commit ac3b43283923 ("module: replace module_layout with module_memory")
introduced a set of memory regions for the module layout sharing the
same attributes but didn't update the kmemleak scanned areas which
intended to limit kmemleak scan to sections containing writable data.
This means sections such as .text and .rodata are scanned by kmemleak.

Refine the scanned areas for modules by limiting it to MOD_TEXT and
MOD_INIT_TEXT mod_mem regions.

CC: Song Liu <song@kernel.org>
CC: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

diff --git a/kernel/module/debug_kmemleak.c b/kernel/module/debug_kmemleak.c
index 12a569d361e8..b4cc03842d70 100644
--- a/kernel/module/debug_kmemleak.c
+++ b/kernel/module/debug_kmemleak.c
@@ -12,19 +12,9 @@
 void kmemleak_load_module(const struct module *mod,
 			  const struct load_info *info)
 {
-	unsigned int i;
-
-	/* only scan the sections containing data */
-	kmemleak_scan_area(mod, sizeof(struct module), GFP_KERNEL);
-
-	for (i = 1; i < info->hdr->e_shnum; i++) {
-		/* Scan all writable sections that's not executable */
-		if (!(info->sechdrs[i].sh_flags & SHF_ALLOC) ||
-		    !(info->sechdrs[i].sh_flags & SHF_WRITE) ||
-		    (info->sechdrs[i].sh_flags & SHF_EXECINSTR))
-			continue;
-
-		kmemleak_scan_area((void *)info->sechdrs[i].sh_addr,
-				   info->sechdrs[i].sh_size, GFP_KERNEL);
+	/* only scan writable, non-executable sections */
+	for_each_mod_mem_type(type) {
+		if (type != MOD_DATA && type != MOD_INIT_DATA)
+			kmemleak_no_scan(mod->mem[type].base);
 	}
 }

base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
-- 
2.46.0.598.g6f2099f65c-goog

Re: [PATCH] module: Refine kmemleak scanned areas

[Luis Chamberlain]

On Fri, Sep 06, 2024 at 04:38:56PM +0100, Vincent Donnefort wrote:
> commit ac3b43283923 ("module: replace module_layout with module_memory")
> introduced a set of memory regions for the module layout sharing the
> same attributes but didn't update the kmemleak scanned areas which
> intended to limit kmemleak scan to sections containing writable data.
> This means sections such as .text and .rodata are scanned by kmemleak.
> 
> Refine the scanned areas for modules by limiting it to MOD_TEXT and
> MOD_INIT_TEXT mod_mem regions.
> 
> CC: Song Liu <song@kernel.org>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>


Refine or fix? If a fix, please use a Fixes tag.

  Luis

Re: [PATCH] module: Refine kmemleak scanned areas

[Catalin Marinas]

On Fri, Sep 06, 2024 at 04:38:56PM +0100, Vincent Donnefort wrote:
> commit ac3b43283923 ("module: replace module_layout with module_memory")
> introduced a set of memory regions for the module layout sharing the
> same attributes but didn't update the kmemleak scanned areas which
> intended to limit kmemleak scan to sections containing writable data.
> This means sections such as .text and .rodata are scanned by kmemleak.
> 
> Refine the scanned areas for modules by limiting it to MOD_TEXT and
> MOD_INIT_TEXT mod_mem regions.
> 
> CC: Song Liu <song@kernel.org>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> 
> diff --git a/kernel/module/debug_kmemleak.c b/kernel/module/debug_kmemleak.c
> index 12a569d361e8..b4cc03842d70 100644
> --- a/kernel/module/debug_kmemleak.c
> +++ b/kernel/module/debug_kmemleak.c
> @@ -12,19 +12,9 @@
>  void kmemleak_load_module(const struct module *mod,
>  			  const struct load_info *info)
>  {
> -	unsigned int i;
> -
> -	/* only scan the sections containing data */
> -	kmemleak_scan_area(mod, sizeof(struct module), GFP_KERNEL);
> -
> -	for (i = 1; i < info->hdr->e_shnum; i++) {
> -		/* Scan all writable sections that's not executable */
> -		if (!(info->sechdrs[i].sh_flags & SHF_ALLOC) ||
> -		    !(info->sechdrs[i].sh_flags & SHF_WRITE) ||
> -		    (info->sechdrs[i].sh_flags & SHF_EXECINSTR))
> -			continue;
> -
> -		kmemleak_scan_area((void *)info->sechdrs[i].sh_addr,
> -				   info->sechdrs[i].sh_size, GFP_KERNEL);
> +	/* only scan writable, non-executable sections */
> +	for_each_mod_mem_type(type) {
> +		if (type != MOD_DATA && type != MOD_INIT_DATA)
> +			kmemleak_no_scan(mod->mem[type].base);
>  	}
>  }

I lost track of how module memory allocation works. Is struct module
still scanned after this change?

-- 
Catalin

Re: [PATCH] module: Refine kmemleak scanned areas

[Vincent Donnefort]

On Sat, Sep 07, 2024 at 03:12:13PM +0100, Catalin Marinas wrote:
> On Fri, Sep 06, 2024 at 04:38:56PM +0100, Vincent Donnefort wrote:
> > commit ac3b43283923 ("module: replace module_layout with module_memory")
> > introduced a set of memory regions for the module layout sharing the
> > same attributes but didn't update the kmemleak scanned areas which
> > intended to limit kmemleak scan to sections containing writable data.
> > This means sections such as .text and .rodata are scanned by kmemleak.
> > 
> > Refine the scanned areas for modules by limiting it to MOD_TEXT and
> > MOD_INIT_TEXT mod_mem regions.
> > 
> > CC: Song Liu <song@kernel.org>
> > CC: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > 
> > diff --git a/kernel/module/debug_kmemleak.c b/kernel/module/debug_kmemleak.c
> > index 12a569d361e8..b4cc03842d70 100644
> > --- a/kernel/module/debug_kmemleak.c
> > +++ b/kernel/module/debug_kmemleak.c
> > @@ -12,19 +12,9 @@
> >  void kmemleak_load_module(const struct module *mod,
> >  			  const struct load_info *info)
> >  {
> > -	unsigned int i;
> > -
> > -	/* only scan the sections containing data */
> > -	kmemleak_scan_area(mod, sizeof(struct module), GFP_KERNEL);
> > -
> > -	for (i = 1; i < info->hdr->e_shnum; i++) {
> > -		/* Scan all writable sections that's not executable */
> > -		if (!(info->sechdrs[i].sh_flags & SHF_ALLOC) ||
> > -		    !(info->sechdrs[i].sh_flags & SHF_WRITE) ||
> > -		    (info->sechdrs[i].sh_flags & SHF_EXECINSTR))
> > -			continue;
> > -
> > -		kmemleak_scan_area((void *)info->sechdrs[i].sh_addr,
> > -				   info->sechdrs[i].sh_size, GFP_KERNEL);
> > +	/* only scan writable, non-executable sections */
> > +	for_each_mod_mem_type(type) {
> > +		if (type != MOD_DATA && type != MOD_INIT_DATA)
> > +			kmemleak_no_scan(mod->mem[type].base);
> >  	}
> >  }
> 
> I lost track of how module memory allocation works. Is struct module
> still scanned after this change?

That section being RW, it will be part of the MOD_DATA vmalloc and scanned.

-- 
Vincent

> 
> -- 
> Catalin

Re: [PATCH] module: Refine kmemleak scanned areas

[Catalin Marinas]

On Mon, Sep 09, 2024 at 08:40:34AM +0100, Vincent Donnefort wrote:
> On Sat, Sep 07, 2024 at 03:12:13PM +0100, Catalin Marinas wrote:
> > On Fri, Sep 06, 2024 at 04:38:56PM +0100, Vincent Donnefort wrote:
> > > commit ac3b43283923 ("module: replace module_layout with module_memory")
> > > introduced a set of memory regions for the module layout sharing the
> > > same attributes but didn't update the kmemleak scanned areas which
> > > intended to limit kmemleak scan to sections containing writable data.
> > > This means sections such as .text and .rodata are scanned by kmemleak.
> > > 
> > > Refine the scanned areas for modules by limiting it to MOD_TEXT and
> > > MOD_INIT_TEXT mod_mem regions.
> > > 
> > > CC: Song Liu <song@kernel.org>
> > > CC: Catalin Marinas <catalin.marinas@arm.com>
> > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > > 
> > > diff --git a/kernel/module/debug_kmemleak.c b/kernel/module/debug_kmemleak.c
> > > index 12a569d361e8..b4cc03842d70 100644
> > > --- a/kernel/module/debug_kmemleak.c
> > > +++ b/kernel/module/debug_kmemleak.c
> > > @@ -12,19 +12,9 @@
> > >  void kmemleak_load_module(const struct module *mod,
> > >  			  const struct load_info *info)
> > >  {
> > > -	unsigned int i;
> > > -
> > > -	/* only scan the sections containing data */
> > > -	kmemleak_scan_area(mod, sizeof(struct module), GFP_KERNEL);
> > > -
> > > -	for (i = 1; i < info->hdr->e_shnum; i++) {
> > > -		/* Scan all writable sections that's not executable */
> > > -		if (!(info->sechdrs[i].sh_flags & SHF_ALLOC) ||
> > > -		    !(info->sechdrs[i].sh_flags & SHF_WRITE) ||
> > > -		    (info->sechdrs[i].sh_flags & SHF_EXECINSTR))
> > > -			continue;
> > > -
> > > -		kmemleak_scan_area((void *)info->sechdrs[i].sh_addr,
> > > -				   info->sechdrs[i].sh_size, GFP_KERNEL);
> > > +	/* only scan writable, non-executable sections */
> > > +	for_each_mod_mem_type(type) {
> > > +		if (type != MOD_DATA && type != MOD_INIT_DATA)
> > > +			kmemleak_no_scan(mod->mem[type].base);
> > >  	}
> > >  }
> > 
> > I lost track of how module memory allocation works. Is struct module
> > still scanned after this change?
> 
> That section being RW, it will be part of the MOD_DATA vmalloc and scanned.

Ah, makes sense. I'm fine with this patch, it simplifies the code now
that we have mod->mem[type]. I wouldn't say it's a fix, though no
backporting needed.

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

Re: [PATCH] module: Refine kmemleak scanned areas

[Vincent Donnefort]

On Mon, Sep 09, 2024 at 09:52:57AM +0100, Catalin Marinas wrote:
> On Mon, Sep 09, 2024 at 08:40:34AM +0100, Vincent Donnefort wrote:
> > On Sat, Sep 07, 2024 at 03:12:13PM +0100, Catalin Marinas wrote:
> > > On Fri, Sep 06, 2024 at 04:38:56PM +0100, Vincent Donnefort wrote:
> > > > commit ac3b43283923 ("module: replace module_layout with module_memory")
> > > > introduced a set of memory regions for the module layout sharing the
> > > > same attributes but didn't update the kmemleak scanned areas which
> > > > intended to limit kmemleak scan to sections containing writable data.
> > > > This means sections such as .text and .rodata are scanned by kmemleak.
> > > > 
> > > > Refine the scanned areas for modules by limiting it to MOD_TEXT and
> > > > MOD_INIT_TEXT mod_mem regions.
> > > > 
> > > > CC: Song Liu <song@kernel.org>
> > > > CC: Catalin Marinas <catalin.marinas@arm.com>
> > > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > > > 
> > > > diff --git a/kernel/module/debug_kmemleak.c b/kernel/module/debug_kmemleak.c
> > > > index 12a569d361e8..b4cc03842d70 100644
> > > > --- a/kernel/module/debug_kmemleak.c
> > > > +++ b/kernel/module/debug_kmemleak.c
> > > > @@ -12,19 +12,9 @@
> > > >  void kmemleak_load_module(const struct module *mod,
> > > >  			  const struct load_info *info)
> > > >  {
> > > > -	unsigned int i;
> > > > -
> > > > -	/* only scan the sections containing data */
> > > > -	kmemleak_scan_area(mod, sizeof(struct module), GFP_KERNEL);
> > > > -
> > > > -	for (i = 1; i < info->hdr->e_shnum; i++) {
> > > > -		/* Scan all writable sections that's not executable */
> > > > -		if (!(info->sechdrs[i].sh_flags & SHF_ALLOC) ||
> > > > -		    !(info->sechdrs[i].sh_flags & SHF_WRITE) ||
> > > > -		    (info->sechdrs[i].sh_flags & SHF_EXECINSTR))
> > > > -			continue;
> > > > -
> > > > -		kmemleak_scan_area((void *)info->sechdrs[i].sh_addr,
> > > > -				   info->sechdrs[i].sh_size, GFP_KERNEL);
> > > > +	/* only scan writable, non-executable sections */
> > > > +	for_each_mod_mem_type(type) {
> > > > +		if (type != MOD_DATA && type != MOD_INIT_DATA)
> > > > +			kmemleak_no_scan(mod->mem[type].base);
> > > >  	}
> > > >  }
> > > 
> > > I lost track of how module memory allocation works. Is struct module
> > > still scanned after this change?
> > 
> > That section being RW, it will be part of the MOD_DATA vmalloc and scanned.
> 
> Ah, makes sense. I'm fine with this patch, it simplifies the code now
> that we have mod->mem[type]. I wouldn't say it's a fix, though no
> backporting needed.

Agreed, it's "fixing" because it was scanning unecessary regions, but it's not
worth any backport.

Aside, judging by the size of this function, I am not sure it's worth to keep
a separated file, but the patch that introduced it didn't really explain why so
I kept it that way.

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

Cheers!

-- 
Vincent

Re: [PATCH] module: Refine kmemleak scanned areas

[Luis Chamberlain]

On Mon, Sep 09, 2024 at 11:23:56AM +0100, Vincent Donnefort wrote:
> On Mon, Sep 09, 2024 at 09:52:57AM +0100, Catalin Marinas wrote:
> > On Mon, Sep 09, 2024 at 08:40:34AM +0100, Vincent Donnefort wrote:
> > > On Sat, Sep 07, 2024 at 03:12:13PM +0100, Catalin Marinas wrote:
> > > > On Fri, Sep 06, 2024 at 04:38:56PM +0100, Vincent Donnefort wrote:
> > > > > commit ac3b43283923 ("module: replace module_layout with module_memory")
> > > > > introduced a set of memory regions for the module layout sharing the
> > > > > same attributes but didn't update the kmemleak scanned areas which
> > > > > intended to limit kmemleak scan to sections containing writable data.
> > > > > This means sections such as .text and .rodata are scanned by kmemleak.
> > > > > 
> > > > > Refine the scanned areas for modules by limiting it to MOD_TEXT and
> > > > > MOD_INIT_TEXT mod_mem regions.
> > > > > 
> > > > > CC: Song Liu <song@kernel.org>
> > > > > CC: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > > > > 
> > > > > diff --git a/kernel/module/debug_kmemleak.c b/kernel/module/debug_kmemleak.c
> > > > > index 12a569d361e8..b4cc03842d70 100644
> > > > > --- a/kernel/module/debug_kmemleak.c
> > > > > +++ b/kernel/module/debug_kmemleak.c
> > > > > @@ -12,19 +12,9 @@
> > > > >  void kmemleak_load_module(const struct module *mod,
> > > > >  			  const struct load_info *info)
> > > > >  {
> > > > > -	unsigned int i;
> > > > > -
> > > > > -	/* only scan the sections containing data */
> > > > > -	kmemleak_scan_area(mod, sizeof(struct module), GFP_KERNEL);
> > > > > -
> > > > > -	for (i = 1; i < info->hdr->e_shnum; i++) {
> > > > > -		/* Scan all writable sections that's not executable */
> > > > > -		if (!(info->sechdrs[i].sh_flags & SHF_ALLOC) ||
> > > > > -		    !(info->sechdrs[i].sh_flags & SHF_WRITE) ||
> > > > > -		    (info->sechdrs[i].sh_flags & SHF_EXECINSTR))
> > > > > -			continue;
> > > > > -
> > > > > -		kmemleak_scan_area((void *)info->sechdrs[i].sh_addr,
> > > > > -				   info->sechdrs[i].sh_size, GFP_KERNEL);
> > > > > +	/* only scan writable, non-executable sections */
> > > > > +	for_each_mod_mem_type(type) {
> > > > > +		if (type != MOD_DATA && type != MOD_INIT_DATA)
> > > > > +			kmemleak_no_scan(mod->mem[type].base);
> > > > >  	}
> > > > >  }
> > > > 
> > > > I lost track of how module memory allocation works. Is struct module
> > > > still scanned after this change?
> > > 
> > > That section being RW, it will be part of the MOD_DATA vmalloc and scanned.
> > 
> > Ah, makes sense. I'm fine with this patch, it simplifies the code now
> > that we have mod->mem[type]. I wouldn't say it's a fix, though no
> > backporting needed.
> 
> Agreed, it's "fixing" because it was scanning unecessary regions, but it's not
> worth any backport.

Please send a v2.

  Luis