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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJuCfpEmqtcrvuSxTzkt28Yf+x4mfDKfWCH_25H+4uDHXaie6g@mail.gmail.com>
Date: Wed, 21 May 2025 01:22:03 +0000
From: Suren Baghdasaryan <surenb@...gle.com>
To: Casey Chen <cachen@...estorage.com>
Cc: 00107082@....com, akpm@...ux-foundation.org, cl@...two.org, 
	dennis@...nel.org, kent.overstreet@...ux.dev, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, pasha.tatashin@...een.com, tj@...nel.org, 
	yzhong@...estorage.com
Subject: Re: comments on patch "alloc_tag: allocate percpu counters for module
 tags dynamically"

On Wed, May 21, 2025 at 12:45 AM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Tue, May 20, 2025 at 11:48 PM Casey Chen <cachen@...estorage.com> wrote:
> >
> > On Tue, May 20, 2025 at 4:26 PM Suren Baghdasaryan <surenb@...gle.com> wrote:
> > >
> > > On Tue, May 20, 2025 at 4:16 PM Casey Chen <cachen@...estorage.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.

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