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

[Suren Baghdasaryan <surenb@google.com>]

On Tue, May 20, 2025 at 6:22 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, May 21, 2025 at 12:45 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > 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.
>
> After inspecting the code some more I'm leaning towards a simpler
> solution of letting codetag_load_module() to return failure and
> handling it with codetag_free_module_sections(). This would avoid
> introducing additional hook inside move_module(). I'll wait until
> tomorrow and if no objections received will post a fixup to my
> previous patch.

Posted the fix at
https://lore.kernel.org/all/20250521160602.1940771-1-surenb@google.com/

>
> >
> > > 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