[v6,1/5] module: Add empty modalias sysfs attribute

This adds the modalias sysfs attribute in preparation for its
implementation.

Signed-off-by: Allen Webb <allenwebb@google.com>
---
 include/linux/module.h   |  1 +
 kernel/module/internal.h |  2 ++
 kernel/module/sysfs.c    | 33 +++++++++++++++++++++++++++++++++
 kernel/params.c          |  7 +++++++
 4 files changed, 43 insertions(+)

Le 02/12/2022 à 23:47, Allen Webb a écrit :
> This adds the modalias sysfs attribute in preparation for its
> implementation.
> 
> Signed-off-by: Allen Webb <allenwebb@google.com>
> ---
>   include/linux/module.h   |  1 +
>   kernel/module/internal.h |  2 ++
>   kernel/module/sysfs.c    | 33 +++++++++++++++++++++++++++++++++
>   kernel/params.c          |  7 +++++++
>   4 files changed, 43 insertions(+)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index ec61fb53979a9..0bfa859a21566 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -47,6 +47,7 @@ struct module_kobject {
>   	struct kobject *drivers_dir;
>   	struct module_param_attrs *mp;
>   	struct completion *kobj_completion;
> +	struct bin_attribute modalias_attr;
>   } __randomize_layout;
>   
>   struct module_attribute {
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 2e2bf236f5582..8d7ae37584868 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -259,11 +259,13 @@ static inline void add_kallsyms(struct module *mod, const struct load_info *info
>   #endif /* CONFIG_KALLSYMS */
>   
>   #ifdef CONFIG_SYSFS
> +void add_modalias_attr(struct module_kobject *mk);
>   int mod_sysfs_setup(struct module *mod, const struct load_info *info,
>   		    struct kernel_param *kparam, unsigned int num_params);
>   void mod_sysfs_teardown(struct module *mod);
>   void init_param_lock(struct module *mod);
>   #else /* !CONFIG_SYSFS */
> +static inline void add_modalias_attr(struct module_kobject *mk) {}
>   static inline int mod_sysfs_setup(struct module *mod,
>   			   	  const struct load_info *info,
>   			   	  struct kernel_param *kparam,
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index ce68f821dcd12..8dafec7455fbe 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -240,6 +240,37 @@ static inline void add_notes_attrs(struct module *mod, const struct load_info *i
>   static inline void remove_notes_attrs(struct module *mod) { }
>   #endif /* CONFIG_KALLSYMS */
>   
> +static ssize_t module_modalias_read(struct file *filp, struct kobject *kobj,
> +				    struct bin_attribute *bin_attr,
> +				    char *buf, loff_t pos, size_t count)
> +{
> +	return 0;
> +}
> +
> +/* Used in kernel/params.c for builtin modules.
> + *
> + * `struct module_kobject` is used instead of `struct module` because for
> + * builtin modules, the `struct module` is not available when this is called.
> + */

You are using network comment style. Other parts of the kernel have 
different style. See 
https://docs.kernel.org/process/coding-style.html#commenting

> +void add_modalias_attr(struct module_kobject *mk)
> +{
> +	sysfs_bin_attr_init(&mk->modalias_attr);
> +	mk->modalias_attr.attr.name = "modalias";
> +	mk->modalias_attr.attr.mode = 0444;
> +	mk->modalias_attr.read = module_modalias_read;
> +	if (sysfs_create_bin_file(&mk->kobj, &mk->modalias_attr)) {
> +		/* We shouldn't ignore the return type, but there is nothing to
> +		 * do.
> +		 */
> +		return;
> +	}

I would have put the comment before the if, it would have been a single 
line comment and you would have avoided the { }

> +}
> +
> +static void remove_modalias_attr(struct module_kobject *mk)
> +{
> +	sysfs_remove_bin_file(&mk->kobj, &mk->modalias_attr);
> +}
> +
>   static void del_usage_links(struct module *mod)
>   {
>   #ifdef CONFIG_MODULE_UNLOAD
> @@ -398,6 +429,7 @@ int mod_sysfs_setup(struct module *mod,
>   
>   	add_sect_attrs(mod, info);
>   	add_notes_attrs(mod, info);
> +	add_modalias_attr(&mod->mkobj);
>   
>   	return 0;
>   
> @@ -415,6 +447,7 @@ int mod_sysfs_setup(struct module *mod,
>   
>   static void mod_sysfs_fini(struct module *mod)
>   {
> +	remove_modalias_attr(&mod->mkobj);
>   	remove_notes_attrs(mod);
>   	remove_sect_attrs(mod);
>   	mod_kobject_put(mod);
> diff --git a/kernel/params.c b/kernel/params.c
> index 5b92310425c50..b7fd5313a3118 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -14,6 +14,12 @@
>   #include <linux/ctype.h>
>   #include <linux/security.h>
>   
> +#ifdef CONFIG_MODULES
> +#include "module/internal.h"
> +#else
> +static inline void add_modalias_attr(struct module_kobject *mk) {}
> +#endif /* !CONFIG_MODULES */

It is odd to include module internal.h outside of module.

If you really need a function from module, I think you should move its 
prototype to include/linux/module.h

> +
>   #ifdef CONFIG_SYSFS
>   /* Protects all built-in parameters, modules use their own param_lock */
>   static DEFINE_MUTEX(param_lock);
> @@ -815,6 +821,7 @@ static void __init kernel_add_sysfs_param(const char *name,
>   	BUG_ON(err);
>   	kobject_uevent(&mk->kobj, KOBJ_ADD);
>   	kobject_put(&mk->kobj);
> +	add_modalias_attr(mk);
>   }
>   
>   /*

On Fri, Dec 02, 2022 at 04:47:40PM -0600, Allen Webb wrote:
> This adds the modalias sysfs attribute in preparation for its
> implementation.

What implementation?

This changelog doesn't make too much sense on it's own, and that's not
good.  Please explain what this attribute is for and what it is going to
do, AND include the documentation for the attribute please.


> 
> Signed-off-by: Allen Webb <allenwebb@google.com>
> ---
>  include/linux/module.h   |  1 +
>  kernel/module/internal.h |  2 ++
>  kernel/module/sysfs.c    | 33 +++++++++++++++++++++++++++++++++
>  kernel/params.c          |  7 +++++++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index ec61fb53979a9..0bfa859a21566 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -47,6 +47,7 @@ struct module_kobject {
>  	struct kobject *drivers_dir;
>  	struct module_param_attrs *mp;
>  	struct completion *kobj_completion;
> +	struct bin_attribute modalias_attr;
>  } __randomize_layout;
>  
>  struct module_attribute {
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 2e2bf236f5582..8d7ae37584868 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -259,11 +259,13 @@ static inline void add_kallsyms(struct module *mod, const struct load_info *info
>  #endif /* CONFIG_KALLSYMS */
>  
>  #ifdef CONFIG_SYSFS
> +void add_modalias_attr(struct module_kobject *mk);

This can not fail?

> +static ssize_t module_modalias_read(struct file *filp, struct kobject *kobj,
> +				    struct bin_attribute *bin_attr,
> +				    char *buf, loff_t pos, size_t count)
> +{
> +	return 0;
> +}
> +
> +/* Used in kernel/params.c for builtin modules.
> + *
> + * `struct module_kobject` is used instead of `struct module` because for
> + * builtin modules, the `struct module` is not available when this is called.
> + */
> +void add_modalias_attr(struct module_kobject *mk)
> +{
> +	sysfs_bin_attr_init(&mk->modalias_attr);
> +	mk->modalias_attr.attr.name = "modalias";
> +	mk->modalias_attr.attr.mode = 0444;
> +	mk->modalias_attr.read = module_modalias_read;
> +	if (sysfs_create_bin_file(&mk->kobj, &mk->modalias_attr)) {
> +		/* We shouldn't ignore the return type, but there is nothing to
> +		 * do.
> +		 */

Odd commenting style.

And yes, if this fails, that is NOT good, please fix that up.

But why is this a static attribute like this as part of the
module_kobject structure and not just a "normal" attribute that is
shared by all kobjects?


> +		return;
> +	}
> +}
> +
> +static void remove_modalias_attr(struct module_kobject *mk)
> +{
> +	sysfs_remove_bin_file(&mk->kobj, &mk->modalias_attr);

Why?  Isn't this automatically cleaned up by the kobject core?


> +}
> +
>  static void del_usage_links(struct module *mod)
>  {
>  #ifdef CONFIG_MODULE_UNLOAD
> @@ -398,6 +429,7 @@ int mod_sysfs_setup(struct module *mod,
>  
>  	add_sect_attrs(mod, info);
>  	add_notes_attrs(mod, info);
> +	add_modalias_attr(&mod->mkobj);

Again, error checking :(

>  
>  	return 0;
>  
> @@ -415,6 +447,7 @@ int mod_sysfs_setup(struct module *mod,
>  
>  static void mod_sysfs_fini(struct module *mod)
>  {
> +	remove_modalias_attr(&mod->mkobj);
>  	remove_notes_attrs(mod);
>  	remove_sect_attrs(mod);
>  	mod_kobject_put(mod);
> diff --git a/kernel/params.c b/kernel/params.c
> index 5b92310425c50..b7fd5313a3118 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -14,6 +14,12 @@
>  #include <linux/ctype.h>
>  #include <linux/security.h>
>  
> +#ifdef CONFIG_MODULES
> +#include "module/internal.h"
> +#else
> +static inline void add_modalias_attr(struct module_kobject *mk) {}
> +#endif /* !CONFIG_MODULES */

#ifdef does not belong in .c files, and mucking around in kernel/params
to go into module/internal.h feels like a layering problem.

> +
>  #ifdef CONFIG_SYSFS
>  /* Protects all built-in parameters, modules use their own param_lock */
>  static DEFINE_MUTEX(param_lock);
> @@ -815,6 +821,7 @@ static void __init kernel_add_sysfs_param(const char *name,
>  	BUG_ON(err);
>  	kobject_uevent(&mk->kobj, KOBJ_ADD);
>  	kobject_put(&mk->kobj);
> +	add_modalias_attr(mk);

You drop the reference on a kobject and then add a sysfs file to it?

Why is this being called here at all?

thanks,

greg k-h