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

[Casey Chen <cachen@purestorage.com>]

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.

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().
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