Re: [PATCH v10 04/11] module.h: MODULE_DEVICE_TABLE for built-in modules

From: Luis Chamberlain <mcgrof@kernel.org>
To: Allen Webb <allenwebb@google.com>
Cc: "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	gregkh@linuxfoundation.org, christophe.leroy@csgroup.eu,
	nick.alcock@oracle.com
Subject: Re: [PATCH v10 04/11] module.h: MODULE_DEVICE_TABLE for built-in modules
Date: Tue, 23 May 2023 23:44:27 -0700	[thread overview]
Message-ID: <ZG2yS0sj0EgGoFYf@bombadil.infradead.org> (raw)
In-Reply-To: <20230406190030.968972-5-allenwebb@google.com>

On Thu, Apr 06, 2023 at 02:00:23PM -0500, Allen Webb wrote:
> Implement MODULE_DEVICE_TABLE for build-in modules to make it possible
> to generate a builtin.alias file to complement modules.alias.
> 
> Signed-off-by: Allen Webb <allenwebb@google.com>
> ---
>  include/linux/module.h | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 4435ad9439ab..b1cb12e06996 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -237,14 +237,36 @@ extern void cleanup_module(void);
>  /* What your module does. */
>  #define MODULE_DESCRIPTION(_description) MODULE_INFO(description, _description)
>  
> -#ifdef MODULE
> -/* Creates an alias so file2alias.c can find device table. */
> +/*
> + * Creates an alias so file2alias.c can find device table.
> + *
> + * Use this in cases where a device table is used to match devices because it
> + * surfaces match-id based module aliases to userspace for:
> + *   - Automatic module loading through modules.alias.
> + *   - Tools like USBGuard which block devices based on policy such as which
> + *     modules match a device.
> + *
> + * The only use-case for built-in drivers today is to enable userspace to
> + * prevent / allow probe for devices on certain subsystems even if the driver is
> + * already loaded. An example is the USB subsystem with its authorized_default
> + * sysfs attribute. For more details refer to the kernel's Documentation for USB
> + * about authorized_default.
> + *
> + * The module name is included in the alias for two reasons:
> + *   - It avoids creating two aliases with the same name for built-in modules.
> + *     Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
> + *     there was nothing to stop different modules from having the same device
> + *     table name and consequently the same alias when building as a module.
> + *   - The module name is needed by files2alias.c to associate a particular
> + *     device table with its associated module for built-in modules since
> + *     files2alias would otherwise see the module name as `vmlinuz.o`.
> + */
>  #define MODULE_DEVICE_TABLE(type, name)					\
> -extern typeof(name) __mod_##type##__##name##_device_table		\
> -  __attribute__ ((unused, alias(__stringify(name))))
> -#else  /* !MODULE */
> -#define MODULE_DEVICE_TABLE(type, name)
> -#endif
> +extern void *CONCATENATE(						\
> +	CONCATENATE(__mod_##type##__##name##__,				\
> +		__KBUILD_MODNAME),					\
> +	_device_table)							\
> +	__attribute__ ((unused, alias(__stringify(name))))

Why does it seem like we're changing extern typeof(name) to a void *?
Also the addition of CONCATENATE() makes it not clear if you are
modifying the definition before, so it would be good to first add
CONCATENATE() to replace the old way without making any functional
changes first. Then a secondary patch which extends the world for
built-in.

  Luis