Re: [RFC][PATCH 0/8] module: Strict per-modname namespaces

[Sean Christopherson <seanjc@google.com>]

On Tue, Nov 12, 2024, Peter Zijlstra wrote:
> On Mon, Nov 11, 2024 at 04:48:58PM -0800, Sean Christopherson wrote:
> > On Mon, Nov 11, 2024, Peter Zijlstra wrote:
> > > Hi!
> > > 
> > > Implement a means for exports to be available only to an explicit list of named
> > > modules. By explicitly limiting the usage of certain exports, the abuse
> > > potential/risk is greatly reduced.
> > > 
> > > The first three 'patches' clean up the existing export namespace code along the
> > > same lines of 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
> > > to __section("foo")") and for the same reason, it is not desired for the
> > > namespace argument to be a macro expansion itself.
> > > 
> > > In fact, the second patch is really only a script, because sending the output
> > > to the list is a giant waste of bandwidth. Whoever eventually commits this to a
> > > git tree should squash these first three patches.
> > > 
> > > The remainder of the patches introduce the special "MODULE_<modname-list>"
> > > namespace, which shall be forbidden from being explicitly imported. A module
> > > that matches the simple modname-list will get an implicit import.
> > > 
> > > Lightly tested with something like:
> > > 
> > > git grep -l EXPORT_SYMBOL arch/x86/kvm/ | while read file;
> > > do
> > >   sed -i -e 's/EXPORT_SYMBOL_GPL(\(.[^)]*\))/EXPORT_SYMBOL_GPL_FOR(\1, "kvm,kvm-intel,kvm-amd")/g' $file;
> > > done
> > 
> > Heh, darn modules.  This will compile just fine, but if the module contains a
> > dash, loading the module will fail because scripts/Makefile.lib replaces the dash
> > with an underscore the build name.  E.g. "kvm-intel" at compile time generates
> > kvm-intel.ko, but the actual name of the module as seen by the kernel is kvm_intel.
> 
> I was wondering about that...  WTH is kvm doing that?

No idea.  The naming has been that way since KVM's inception in commit 6aa8b732ca01
("[PATCH] kvm: userspace interface").  My guess is that either no one noticed, or
those who noticed didn't care.

FWIW, IMO the kernel build system is the one that's being weird.  AFAICT, the
'-' => '_' conversion was added so that spinlocks could be placed into unique
subsections.  Amusingly, it doesn't appear that there are any remaining users of
LOCK_SECTION_NAME.

  commit b5635319d32438ed516568f53013a460ba16e6ee
  Author:     Dave Jones <davej@suse.de>
  AuthorDate: Fri Feb 8 01:43:23 2002 -0800
  Commit:     Linus Torvalds <torvalds@penguin.transmeta.com>
  CommitDate: Fri Feb 8 01:43:23 2002 -0800

    [PATCH] text.lock -> subsection changes.
    
    Make spinlocks etc use subsections of their parent sections instead of
    an ELF section of their own - needed for newer binutils when the parent
    sector is removed.

#define LOCK_SECTION_NAME ".text..lock."KBUILD_BASENAME

#define LOCK_SECTION_START(extra)               \
        ".subsection 1\n\t"                     \
        extra                                   \
        ".ifndef " LOCK_SECTION_NAME "\n\t"     \
        LOCK_SECTION_NAME ":\n\t"               \
        ".endif\n"

#define LOCK_SECTION_END                        \
        ".previous\n\t"

#define __lockfunc __section(".spinlock.text")


> I mean, I suppose you can do: "kvm-intel,kvm_intel" but that's somewhat
> tedious.

This likely needs to be addressed in whatever chunk of code is enforcing the
namespaces.  The s/-/_ behavior (and vice versa!) is *very* baked into the kernel
at this point, e.g. parameqn() will happily parse dashes or underscores for every
kernel parameter.  As horrific as it is, I think the module namespace needs to do
the same, i.e. treat dashes and underscores as one and the same.


More historical amusement:

commit 8863179c65618844379ef90d4a708293042465c8
Author:     Andrew Morton <akpm@digeo.com>
AuthorDate: Sun Feb 2 06:08:27 2003 -0800
Commit:     Linus Torvalds <torvalds@home.transmeta.com>
CommitDate: Sun Feb 2 06:08:27 2003 -0800

    [PATCH] kernel param and KBUILD_MODNAME name-munging mess
    
    Patch from: Rusty Russell <rusty@rustcorp.com.au>
    
    Mikael Pettersson points out that "-s" gets mangled to "_s" on the
    kernel command line, even though it turns out not to be a
    parameter.

commit 326e7842d30d5cfc1089b85a7aa63e5c9f3c0a74
Author:     Rusty Russell <rusty@rustcorp.com.au>
AuthorDate: Sat Dec 14 20:13:11 2002 -0800
Commit:     Linus Torvalds <torvalds@home.transmeta.com>
CommitDate: Sat Dec 14 20:13:11 2002 -0800

    [PATCH] Module Parameter Core Patch
    
    This patch is a rewrite of the insmod and boot parameter handling,
    to unify them.
    
    The new format is fairly simple: built on top of __module_param_call there
    are several helpers, eg "module_param(foo, int, 000)".  The final argument
    is the permissions bits, for exposing parameters in sysfs (if
    non-zero) at a later stage.