diff mbox series

[v2,1/6] module: fix kmemleak annotations for non init ELF sections

Message ID 20230405022702.753323-2-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show
Series module: avoid userspace pressure on unwanted allocations | expand

Commit Message

Luis Chamberlain April 5, 2023, 2:26 a.m. UTC
Commit ac3b43283923 ("module: replace module_layout with module_memory")
reworked the way to handle memory allocations to make it clearer. But it
lost in translation how we handled kmemleak_ignore() or kmemleak_not_leak()
for different ELF sections.

Fix this and clarify the comments a bit more.

Fixes: ac3b43283923 ("module: replace module_layout with module_memory")
Reported-by: Jim Cromie <jim.cromie@gmail.com>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/module/main.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Song Liu April 5, 2023, 6:52 a.m. UTC | #1
On Tue, Apr 4, 2023 at 7:27 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> Commit ac3b43283923 ("module: replace module_layout with module_memory")
> reworked the way to handle memory allocations to make it clearer. But it
> lost in translation how we handled kmemleak_ignore() or kmemleak_not_leak()
> for different ELF sections.
>
> Fix this and clarify the comments a bit more.
>
> Fixes: ac3b43283923 ("module: replace module_layout with module_memory")
> Reported-by: Jim Cromie <jim.cromie@gmail.com>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

Acked-by: Song Liu <song@kernel.org>

Thanks for the fix!

> ---
>  kernel/module/main.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 5cc21083af04..d8bb23fa6989 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2233,11 +2233,23 @@ static int move_module(struct module *mod, struct load_info *info)
>                 ptr = module_memory_alloc(mod->mem[type].size, type);
>
>                 /*
> -                * The pointer to this block is stored in the module structure
> -                * which is inside the block. Just mark it as not being a
> -                * leak.
> +                * The pointer to these blocks of memory are stored on the module
> +                * structure and we keep that around so long as the module is
> +                * around. We only free that memory when we unload the module.
> +                * Just mark them as not being a leak then. The .init* ELF
> +                * sections *do* get freed after boot so we treat them slightly
> +                * differently and only grey them out -- they work as typical
> +                * memory allocations which *do* eventually get freed.
>                  */
> -               kmemleak_ignore(ptr);
> +               switch (type) {
> +               case MOD_INIT_TEXT: /* fallthrough */
> +               case MOD_INIT_DATA: /* fallthrough */
> +               case MOD_INIT_RODATA: /* fallthrough */
> +                       kmemleak_ignore(ptr);
> +                       break;
> +               default:
> +                       kmemleak_not_leak(ptr);
> +               }
>                 if (!ptr) {
>                         t = type;
>                         goto out_enomem;
> --
> 2.39.2
>
Catalin Marinas April 11, 2023, 3:17 p.m. UTC | #2
On Tue, Apr 04, 2023 at 07:26:57PM -0700, Luis Chamberlain wrote:
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 5cc21083af04..d8bb23fa6989 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2233,11 +2233,23 @@ static int move_module(struct module *mod, struct load_info *info)
>  		ptr = module_memory_alloc(mod->mem[type].size, type);
>  
>  		/*
> -		 * The pointer to this block is stored in the module structure
> -		 * which is inside the block. Just mark it as not being a
> -		 * leak.
> +		 * The pointer to these blocks of memory are stored on the module
> +		 * structure and we keep that around so long as the module is
> +		 * around. We only free that memory when we unload the module.
> +		 * Just mark them as not being a leak then. The .init* ELF
> +		 * sections *do* get freed after boot so we treat them slightly
> +		 * differently and only grey them out -- they work as typical
> +		 * memory allocations which *do* eventually get freed.
>  		 */
> -		kmemleak_ignore(ptr);
> +		switch (type) {
> +		case MOD_INIT_TEXT: /* fallthrough */
> +		case MOD_INIT_DATA: /* fallthrough */
> +		case MOD_INIT_RODATA: /* fallthrough */
> +			kmemleak_ignore(ptr);
> +			break;
> +		default:
> +			kmemleak_not_leak(ptr);
> +		}

This works as well but if you want to keep it simple, just call
kmemleak_not_leak() in all cases. When freeing the init sections, they
would be removed from the kmemleak tracing anyway.
Luis Chamberlain April 11, 2023, 5:06 p.m. UTC | #3
On Tue, Apr 11, 2023 at 04:17:35PM +0100, Catalin Marinas wrote:
> On Tue, Apr 04, 2023 at 07:26:57PM -0700, Luis Chamberlain wrote:
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index 5cc21083af04..d8bb23fa6989 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -2233,11 +2233,23 @@ static int move_module(struct module *mod, struct load_info *info)
> >  		ptr = module_memory_alloc(mod->mem[type].size, type);
> >  
> >  		/*
> > -		 * The pointer to this block is stored in the module structure
> > -		 * which is inside the block. Just mark it as not being a
> > -		 * leak.
> > +		 * The pointer to these blocks of memory are stored on the module
> > +		 * structure and we keep that around so long as the module is
> > +		 * around. We only free that memory when we unload the module.
> > +		 * Just mark them as not being a leak then. The .init* ELF
> > +		 * sections *do* get freed after boot so we treat them slightly
> > +		 * differently and only grey them out -- they work as typical
> > +		 * memory allocations which *do* eventually get freed.
> >  		 */
> > -		kmemleak_ignore(ptr);
> > +		switch (type) {
> > +		case MOD_INIT_TEXT: /* fallthrough */
> > +		case MOD_INIT_DATA: /* fallthrough */
> > +		case MOD_INIT_RODATA: /* fallthrough */
> > +			kmemleak_ignore(ptr);
> > +			break;
> > +		default:
> > +			kmemleak_not_leak(ptr);
> > +		}
> 
> This works as well but if you want to keep it simple, just call
> kmemleak_not_leak() in all cases. When freeing the init sections, they
> would be removed from the kmemleak tracing anyway.

It is up to you as you were the one who originally used different calls
here, so I didn't want to change the old mechanism. Changing it to use
kmemleak_not_leak() would be a functional change, do we loose anything
for using kmemleak_not_leak() for all? Ie, why had you used a different
set of calls when you first added this depending on the if its init or
not? Is the value no longer there?

  Luis
diff mbox series

Patch

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 5cc21083af04..d8bb23fa6989 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2233,11 +2233,23 @@  static int move_module(struct module *mod, struct load_info *info)
 		ptr = module_memory_alloc(mod->mem[type].size, type);
 
 		/*
-		 * The pointer to this block is stored in the module structure
-		 * which is inside the block. Just mark it as not being a
-		 * leak.
+		 * The pointer to these blocks of memory are stored on the module
+		 * structure and we keep that around so long as the module is
+		 * around. We only free that memory when we unload the module.
+		 * Just mark them as not being a leak then. The .init* ELF
+		 * sections *do* get freed after boot so we treat them slightly
+		 * differently and only grey them out -- they work as typical
+		 * memory allocations which *do* eventually get freed.
 		 */
-		kmemleak_ignore(ptr);
+		switch (type) {
+		case MOD_INIT_TEXT: /* fallthrough */
+		case MOD_INIT_DATA: /* fallthrough */
+		case MOD_INIT_RODATA: /* fallthrough */
+			kmemleak_ignore(ptr);
+			break;
+		default:
+			kmemleak_not_leak(ptr);
+		}
 		if (!ptr) {
 			t = type;
 			goto out_enomem;