Re: comments on patch "alloc_tag: allocate percpu counters for module tags dynamically"

[Suren Baghdasaryan <surenb@google.com>]

On Tue, May 20, 2025 at 11:48 PM Casey Chen <cachen@purestorage.com> wrote:
>
> On Tue, May 20, 2025 at 4:26 PM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Tue, May 20, 2025 at 4:16 PM Casey Chen <cachen@purestorage.com> wrote:
> > >
> > > Hi Suren,
> >
> > Hi Casey,
> >
> > >
> > > I have two questions on this patch.
> > > 1. If load_module() fails to allocate memory for percpu counters, should we call codetag_free_module_sections() to clean up module tags memory ?
> >
> > Does this address your question:
> > https://lore.kernel.org/all/20250518101212.19930-1-00107082@163.com/
> >
>
> module_deallocate() is called in error handling of load_module(). And
> codetag_load_module() is at the very end of load_module(). If counter
> allocation fails, it doesn't go to the error path to clean up module
> tag memory.

Ah, right. I didn't have the code in front of me but now I see what
you mean. codetag_load_module() does not return a fault if percpu
counters fail to allocate.

>
> My code base is at a5806cd506af ("Linux 6.15-rc7")
> 3250 /*
> 3251  * Allocate and load the module: note that size of section 0 is always
> 3252  * zero, and we rely on this for optional sections.
> 3253  */
> 3254 static int load_module(struct load_info *info, const char __user *uargs,
> 3255                        int flags)
> 3256 {
> ...
> 3403
> 3404         codetag_load_module(mod);
> 3405
> 3406         /* Done! */
> 3407         trace_module_load(mod);
> 3408
> 3409         return do_init_module(mod);
> ...
> 3445  free_module:
> 3446         mod_stat_bump_invalid(info, flags);
> 3447         /* Free lock-classes; relies on the preceding sync_rcu() */
> 3448         for_class_mod_mem_type(type, core_data) {
> 3449                 lockdep_free_key_range(mod->mem[type].base,
> 3450                                        mod->mem[type].size);
> 3451         }
> 3452
> 3453         module_memory_restore_rox(mod);
> 3454         module_deallocate(mod, info);
>
>
> > > 2. How about moving percpu counters allocation to move_module() where codetag_alloc_module_section() is called ? So they can be cleaned up together.
> >
> > That would not work because tag->counters are initialized with NULL
> > after move_module() executes, so if we allocate there our allocations
> > will be overridden. We have to do that at the end of load_module()
> > where codetag_load_module() is.
>
> codetag_alloc_module_section() is called in move_module() to allocate
> module tag memory. I mean we can also allocate memory for percpu
> counters inside move_module().

I thought you were suggesting to allocate percpu counters inside
codetag_alloc_module_section(). I guess we could introduce another
callback to allocate these counters at the end of the move_module(). I
think simpler option is to let codetag_load_module() to fail and
handle that failure, OTOH that means that we do more work before
failing... Let me think some more on which way is preferable and I'll
post a fixup to my earlier patch.

> We have implemented such a thing in our code base and it works fine.
> Just do it right after copying ELF sections to memory. If it fails it
> goes to the error path and calls codetag_free_module_sections() to
> clean up.
>
> 2650                 if (codetag_needs_module_section(mod, sname,
> shdr->sh_size)) {
> 2651                         dest = codetag_alloc_module_section(mod,
> sname, shdr->sh_size,
> 2652
> arch_mod_section_prepend(mod, i), shdr->sh_addralign);
>
> > Thanks,
> > Suren.
> >
> > >
> > > Thanks,
> > > Casey