[PATCH] RFC: ksmbd: provide MODULE_VERSION()

[PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Linus Walleij]

Adding MODULE_VERSION("1.0") to ksmbd makes /sys/modules/ksmbd
appear even when ksmbd is compiled into the kernel.

Adding a version as a way to get a module name is documented in
Documentation/ABI/stable/sysfs-module.

This is nice when you boot a custom kernel on OpenWrt because
the startup script does things such as:

[ ! -e /sys/module/ksmbd ] && modprobe ksmbd 2> /dev/null
if [ ! -e /sys/module/ksmbd ]; then
    logger -p daemon.error -t 'ksmbd' "modprobe of ksmbd "
    "module failed, can\'t start ksmbd!"
     exit 1
fi

which makes the script not work with a compiled-in ksmbd.

Since I actually turn off modules and compile all my modules
into the kernel, I can't change the script to just check
cat /lib/modules/$(uname -r)/modules.builtin | grep ksmbd
either: no /lib/modules directory.

We define the string together with the version number for
the netlink, as they probably should be in tandem. Perhaps
this is a general nice thing to do.

An option would be to change the script to proceed and
just assume the module is compiled in but it feels wrong.

Cc: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 fs/smb/server/ksmbd_netlink.h | 1 +
 fs/smb/server/server.c        | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/fs/smb/server/ksmbd_netlink.h b/fs/smb/server/ksmbd_netlink.h
index 3f07a612c05b40a000151cef1117b918dc5da9ea..9ae6be7c7b116ab29277074089f2305e2b243594 100644
--- a/fs/smb/server/ksmbd_netlink.h
+++ b/fs/smb/server/ksmbd_netlink.h
@@ -58,6 +58,7 @@
 
 #define KSMBD_GENL_NAME		"SMBD_GENL"
 #define KSMBD_GENL_VERSION		0x01
+#define KSMBD_GENL_VERSION_STRING	"1.0"
 
 #define KSMBD_REQ_MAX_ACCOUNT_NAME_SZ	48
 #define KSMBD_REQ_MAX_HASH_SZ		18
diff --git a/fs/smb/server/server.c b/fs/smb/server/server.c
index ab533c6029879fe8a972ebc85efc829ef0e0dcd3..332e3e39dc613d8a9077da0745eb1fb7a30b6bb5 100644
--- a/fs/smb/server/server.c
+++ b/fs/smb/server/server.c
@@ -632,5 +632,7 @@ MODULE_SOFTDEP("pre: aead2");
 MODULE_SOFTDEP("pre: ccm");
 MODULE_SOFTDEP("pre: gcm");
 MODULE_SOFTDEP("pre: crc32");
+/* MODULE_VERSION() Makes /sys/module/ksmbd appear when compiled-in */
+MODULE_VERSION(KSMBD_GENL_VERSION_STRING);
 module_init(ksmbd_server_init)
 module_exit(ksmbd_server_exit)

---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250531-ksmbd-sysfs-module-efaf2d1a86ca

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Greg Kroah-Hartman]

On Sat, May 31, 2025 at 08:42:44AM +0200, Linus Walleij wrote:
> Adding MODULE_VERSION("1.0") to ksmbd makes /sys/modules/ksmbd
> appear even when ksmbd is compiled into the kernel.
> 
> Adding a version as a way to get a module name is documented in
> Documentation/ABI/stable/sysfs-module.

MODULE_VERSIONS() mean nothing and should be removed entirely from the
kernel tree.  The only "version" that is an issue is the kernel version.

>  #define KSMBD_GENL_NAME		"SMBD_GENL"
>  #define KSMBD_GENL_VERSION		0x01
> +#define KSMBD_GENL_VERSION_STRING	"1.0"

So you are overloading the module version attribute to somehow reflect
a user/kernel api version number instead?  That feels wrong and not a
good idea.  Export that "version" somewhere else please if userspace
really needs it (and I would strongly argue that it should NOT need it
as you aren't allowed to make breaking user/kernel api changes anyway,
so why would a version number matter?)

> --- a/fs/smb/server/server.c
> +++ b/fs/smb/server/server.c
> @@ -632,5 +632,7 @@ MODULE_SOFTDEP("pre: aead2");
>  MODULE_SOFTDEP("pre: ccm");
>  MODULE_SOFTDEP("pre: gcm");
>  MODULE_SOFTDEP("pre: crc32");
> +/* MODULE_VERSION() Makes /sys/module/ksmbd appear when compiled-in */
> +MODULE_VERSION(KSMBD_GENL_VERSION_STRING);

Again, no, please don't do this.

greg k-h

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Steve French]

It is interesting that almost 700 kernel modules define
MODULE_VERSION() for their module, but only 4 filesystems (including
cifs.ko).  I find it useful mainly for seeing which fixes are in
(since some distros do 'full backports' so easier to look at the
module version sometimes to see what fixes are likely in the module
when someone reports a problem).   I am curious why few fs use it
though since it is apparently very widely used for other module types.

On Sat, May 31, 2025 at 1:42 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Adding MODULE_VERSION("1.0") to ksmbd makes /sys/modules/ksmbd
> appear even when ksmbd is compiled into the kernel.
>
> Adding a version as a way to get a module name is documented in
> Documentation/ABI/stable/sysfs-module.
>
> This is nice when you boot a custom kernel on OpenWrt because
> the startup script does things such as:
>
> [ ! -e /sys/module/ksmbd ] && modprobe ksmbd 2> /dev/null
> if [ ! -e /sys/module/ksmbd ]; then
>     logger -p daemon.error -t 'ksmbd' "modprobe of ksmbd "
>     "module failed, can\'t start ksmbd!"
>      exit 1
> fi
>
> which makes the script not work with a compiled-in ksmbd.
>
> Since I actually turn off modules and compile all my modules
> into the kernel, I can't change the script to just check
> cat /lib/modules/$(uname -r)/modules.builtin | grep ksmbd
> either: no /lib/modules directory.
>
> We define the string together with the version number for
> the netlink, as they probably should be in tandem. Perhaps
> this is a general nice thing to do.
>
> An option would be to change the script to proceed and
> just assume the module is compiled in but it feels wrong.
>
> Cc: Rosen Penev <rosenp@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  fs/smb/server/ksmbd_netlink.h | 1 +
>  fs/smb/server/server.c        | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/fs/smb/server/ksmbd_netlink.h b/fs/smb/server/ksmbd_netlink.h
> index 3f07a612c05b40a000151cef1117b918dc5da9ea..9ae6be7c7b116ab29277074089f2305e2b243594 100644
> --- a/fs/smb/server/ksmbd_netlink.h
> +++ b/fs/smb/server/ksmbd_netlink.h
> @@ -58,6 +58,7 @@
>
>  #define KSMBD_GENL_NAME                "SMBD_GENL"
>  #define KSMBD_GENL_VERSION             0x01
> +#define KSMBD_GENL_VERSION_STRING      "1.0"
>
>  #define KSMBD_REQ_MAX_ACCOUNT_NAME_SZ  48
>  #define KSMBD_REQ_MAX_HASH_SZ          18
> diff --git a/fs/smb/server/server.c b/fs/smb/server/server.c
> index ab533c6029879fe8a972ebc85efc829ef0e0dcd3..332e3e39dc613d8a9077da0745eb1fb7a30b6bb5 100644
> --- a/fs/smb/server/server.c
> +++ b/fs/smb/server/server.c
> @@ -632,5 +632,7 @@ MODULE_SOFTDEP("pre: aead2");
>  MODULE_SOFTDEP("pre: ccm");
>  MODULE_SOFTDEP("pre: gcm");
>  MODULE_SOFTDEP("pre: crc32");
> +/* MODULE_VERSION() Makes /sys/module/ksmbd appear when compiled-in */
> +MODULE_VERSION(KSMBD_GENL_VERSION_STRING);
>  module_init(ksmbd_server_init)
>  module_exit(ksmbd_server_exit)
>
> ---
> base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
> change-id: 20250531-ksmbd-sysfs-module-efaf2d1a86ca
>
> Best regards,
> --
> Linus Walleij <linus.walleij@linaro.org>
>


-- 
Thanks,

Steve

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> It is interesting that almost 700 kernel modules define
> MODULE_VERSION() for their module, but only 4 filesystems (including
> cifs.ko).  I find it useful mainly for seeing which fixes are in
> (since some distros do 'full backports' so easier to look at the
> module version sometimes to see what fixes are likely in the module
> when someone reports a problem).   I am curious why few fs use it
> though since it is apparently very widely used for other module types.

I find cifs.ko version quite useless, especially for distro and stable
kernels which take fixes from newer versions while not backporting the
commit that bumps cifs.ko version.  So relying on that version becomes
pointless, IMO.

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Linus Walleij]

On Sat, May 31, 2025 at 8:56 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, May 31, 2025 at 08:42:44AM +0200, Linus Walleij wrote:

> > Adding MODULE_VERSION("1.0") to ksmbd makes /sys/modules/ksmbd
> > appear even when ksmbd is compiled into the kernel.
> >
> > Adding a version as a way to get a module name is documented in
> > Documentation/ABI/stable/sysfs-module.
>
> MODULE_VERSIONS() mean nothing and should be removed entirely from the
> kernel tree.  The only "version" that is an issue is the kernel version.

I get it, I need to think of some better way to handle this.
(Adding a random module parameter just to get this dir is probably a
bad idea too.)

Yours,
Linus Walleij

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Greg Kroah-Hartman]

On Sat, May 31, 2025 at 03:49:47PM -0300, Paulo Alcantara wrote:
> Steve French <smfrench@gmail.com> writes:
> 
> > It is interesting that almost 700 kernel modules define
> > MODULE_VERSION() for their module, but only 4 filesystems (including
> > cifs.ko).  I find it useful mainly for seeing which fixes are in
> > (since some distros do 'full backports' so easier to look at the
> > module version sometimes to see what fixes are likely in the module
> > when someone reports a problem).   I am curious why few fs use it
> > though since it is apparently very widely used for other module types.
> 
> I find cifs.ko version quite useless, especially for distro and stable
> kernels which take fixes from newer versions while not backporting the
> commit that bumps cifs.ko version.  So relying on that version becomes
> pointless, IMO.

Yes, it is pointless, which is why it really should just be removed.
I'll do a sweep of the tree after -rc1 is out and start sending out
patches...

thanks,

greg k-h

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Linus Walleij]

On Sun, Jun 1, 2025 at 9:41 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, May 31, 2025 at 03:49:47PM -0300, Paulo Alcantara wrote:
> >  So relying on that version becomes
> > pointless, IMO.
>
> Yes, it is pointless, which is why it really should just be removed.
> I'll do a sweep of the tree after -rc1 is out and start sending out
> patches...

In a way I'm happy with that result :D

But what do you think about the original problem of making
the dir /sys/modules/foo appear for a compiled-in module?

So as to detect "functionality of module foo is present".

Is that something we could allow (I have a patch using just
lookup_or_create_module_kobject()) or something userspace
needs to figure out another way?

Yours,
Linus Walleij

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Steve French]

On Sun, Jun 1, 2025 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sun, Jun 1, 2025 at 9:41 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, May 31, 2025 at 03:49:47PM -0300, Paulo Alcantara wrote:
> > >  So relying on that version becomes
> > > pointless, IMO.
> >
> > Yes, it is pointless, which is why it really should just be removed.
> > I'll do a sweep of the tree after -rc1 is out and start sending out
> > patches...
>
> In a way I'm happy with that result :D
>
> But what do you think about the original problem of making
> the dir /sys/modules/foo appear for a compiled-in module?
>
> So as to detect "functionality of module foo is present".

Yes, and also (at least in my example), I find it sometimes helpful to
look at module version number since it can give a rough idea of what
"features" are in that version of the cifs.ko module (especially if
tricky to access the git tree for that kernel build).  Seems harmless
to keep it

-- 
Thanks,

Steve

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Paulo Alcantara]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Sat, May 31, 2025 at 03:49:47PM -0300, Paulo Alcantara wrote:
>> Steve French <smfrench@gmail.com> writes:
>> 
>> > It is interesting that almost 700 kernel modules define
>> > MODULE_VERSION() for their module, but only 4 filesystems (including
>> > cifs.ko).  I find it useful mainly for seeing which fixes are in
>> > (since some distros do 'full backports' so easier to look at the
>> > module version sometimes to see what fixes are likely in the module
>> > when someone reports a problem).   I am curious why few fs use it
>> > though since it is apparently very widely used for other module types.
>> 
>> I find cifs.ko version quite useless, especially for distro and stable
>> kernels which take fixes from newer versions while not backporting the
>> commit that bumps cifs.ko version.  So relying on that version becomes
>> pointless, IMO.
>
> Yes, it is pointless, which is why it really should just be removed.
> I'll do a sweep of the tree after -rc1 is out and start sending out
> patches...

Sounds good.  Kernel version and git tree are just enough to figure out
what a filesystem or driver has in terms of features or fixes.

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Steve French]

On Sun, Jun 1, 2025 at 2:56 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>
> > On Sat, May 31, 2025 at 03:49:47PM -0300, Paulo Alcantara wrote:
> >> Steve French <smfrench@gmail.com> writes:
> >>
> >> > It is interesting that almost 700 kernel modules define
> >> > MODULE_VERSION() for their module, but only 4 filesystems (including
> >> > cifs.ko).  I find it useful mainly for seeing which fixes are in
> >> > (since some distros do 'full backports' so easier to look at the
> >> > module version sometimes to see what fixes are likely in the module
> >> > when someone reports a problem).   I am curious why few fs use it
> >> > though since it is apparently very widely used for other module types.
> >>
> >> I find cifs.ko version quite useless, especially for distro and stable
> >> kernels which take fixes from newer versions while not backporting the
> >> commit that bumps cifs.ko version.  So relying on that version becomes
> >> pointless, IMO.
> >
> > Yes, it is pointless, which is why it really should just be removed.
> > I'll do a sweep of the tree after -rc1 is out and start sending out
> > patches...
>
> Sounds good.  Kernel version and git tree are just enough to figure out
> what a filesystem or driver has in terms of features or fixes.

I ran into weird kernel versions a few times where module version was
useful, so the external "features wiki" for cifs.ko lists both module version
and kernel version in the list of features.

-- 
Thanks,

Steve

Re: [PATCH] RFC: ksmbd: provide MODULE_VERSION()

[Greg Kroah-Hartman]

On Sun, Jun 01, 2025 at 08:54:10PM +0200, Linus Walleij wrote:
> On Sun, Jun 1, 2025 at 9:41 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, May 31, 2025 at 03:49:47PM -0300, Paulo Alcantara wrote:
> > >  So relying on that version becomes
> > > pointless, IMO.
> >
> > Yes, it is pointless, which is why it really should just be removed.
> > I'll do a sweep of the tree after -rc1 is out and start sending out
> > patches...
> 
> In a way I'm happy with that result :D
> 
> But what do you think about the original problem of making
> the dir /sys/modules/foo appear for a compiled-in module?

It does that today, if the module has a parameter, right?  Maybe change
that to always do that even if there is no parameter present and see
what happens?

thanks,

greg k-h