diff mbox series

libkmod: Always search modules.builtin if no alias has been found

Message ID 20210504235836.24246-1-pkj@axis.com (mailing list archive)
State New, archived
Headers show
Series libkmod: Always search modules.builtin if no alias has been found | expand

Commit Message

Peter Kjellerstedt May 4, 2021, 11:58 p.m. UTC
Commit 89443220e broke the lookup for builtin modules. modules.builtin
was no longer searched if kmod_lookup_alias_from_kernel_builtin_file()
returned 0.

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---

I do not know if this is the correct thing to do, or if the commit
message makes any sense. However, it solves the problem we were seeing.
We use fuse, which installs /etc/modules-load.d/fuse.conf to load the
fuse kernel module. However, we have fuse built-in. Normally, the
following can be seen in the log:

  systemd-modules-load[192]: Module 'fuse' is built in

but after commit 89443220e, we instead got:

  systemd-modules-load[193]: Failed to find module 'fuse'

//Peter

 libkmod/libkmod-module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Lucas De Marchi May 9, 2021, 5:55 a.m. UTC | #1
On Tue, May 4, 2021 at 5:17 PM Peter Kjellerstedt <pkj@axis.com> wrote:
>
> Commit 89443220e broke the lookup for builtin modules. modules.builtin
> was no longer searched if kmod_lookup_alias_from_kernel_builtin_file()
> returned 0.
>
> Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> ---
>
> I do not know if this is the correct thing to do, or if the commit
> message makes any sense. However, it solves the problem we were seeing.
> We use fuse, which installs /etc/modules-load.d/fuse.conf to load the
> fuse kernel module. However, we have fuse built-in. Normally, the
> following can be seen in the log:
>
>   systemd-modules-load[192]: Module 'fuse' is built in
>
> but after commit 89443220e, we instead got:
>
>   systemd-modules-load[193]: Failed to find module 'fuse'
>
> //Peter
>
>  libkmod/libkmod-module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 76a6dc3..6720930 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -577,7 +577,7 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
>
>         DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
>         err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> -       if (err == -ENOSYS) {
> +       if (err == 0 || err == -ENOSYS) {

So in your case you do have modules.builtin.modinfo, but fuse doesn't
show up there. On the other hand it is listed in modules.builtin.
Does modules.builtin.info contain anything or is it an empty file?

It seems to me something else is broken:  all modules in
modules.builtin should be in modules.builtin.modinfo as well. What is
the result of the following commands?

grep fuse /lib/modules/$(uname -r)/modules.builtin
grep fuse /lib/modules/$(uname -r)/modules.builtin.modinfo

thanks
Lucas De Marchi

>                 /* Optional index missing, try the old one */
>                 DBG(ctx, "lookup modules.builtin %s\n", alias);
>                 err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
Peter Kjellerstedt May 10, 2021, 1:55 p.m. UTC | #2
> -----Original Message-----
> From: Lucas De Marchi <lucas.de.marchi@gmail.com>
> Sent: den 9 maj 2021 07:55
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> Cc: linux-modules <linux-modules@vger.kernel.org>; Peter Kjellerstedt
> <peter.kjellerstedt@axis.com>
> Subject: Re: [PATCH] libkmod: Always search modules.builtin if no alias
> has been found
> 
> On Tue, May 4, 2021 at 5:17 PM Peter Kjellerstedt <pkj@axis.com> wrote:
> >
> > Commit 89443220e broke the lookup for builtin modules. modules.builtin
> > was no longer searched if kmod_lookup_alias_from_kernel_builtin_file()
> > returned 0.
> >
> > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > ---
> >
> > I do not know if this is the correct thing to do, or if the commit
> > message makes any sense. However, it solves the problem we were seeing.
> > We use fuse, which installs /etc/modules-load.d/fuse.conf to load the
> > fuse kernel module. However, we have fuse built-in. Normally, the
> > following can be seen in the log:
> >
> >   systemd-modules-load[192]: Module 'fuse' is built in
> >
> > but after commit 89443220e, we instead got:
> >
> >   systemd-modules-load[193]: Failed to find module 'fuse'
> >
> > //Peter
> >
> >  libkmod/libkmod-module.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> > index 76a6dc3..6720930 100644
> > --- a/libkmod/libkmod-module.c
> > +++ b/libkmod/libkmod-module.c
> > @@ -577,7 +577,7 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
> >
> >         DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
> >         err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> > -       if (err == -ENOSYS) {
> > +       if (err == 0 || err == -ENOSYS) {
> 
> So in your case you do have modules.builtin.modinfo, but fuse doesn't
> show up there. On the other hand it is listed in modules.builtin.
> Does modules.builtin.info contain anything or is it an empty file?

We have neither modules.builtin.modinfo nor modules.builtin.info.
A little googling turned out that modules.builtin.modinfo seems to have 
been introduced in 5.2, but this product uses a 4.19 based kernel.

> It seems to me something else is broken:  all modules in
> modules.builtin should be in modules.builtin.modinfo as well. What is
> the result of the following commands?
> 
> grep fuse /lib/modules/$(uname -r)/modules.builtin

kernel/fs/fuse/fuse.ko

> grep fuse /lib/modules/$(uname -r)/modules.builtin.modinfo

grep: /lib/modules/4.19.110-axis8/modules.builtin.modinfo: No such file or directory

This is an embedded product built with our own distribution based on 
Poky Gatesgarth from the Yocto Project. The rootfs is read-only, including 
/lib/modules, so any contest there is created when the firmware image is 
built.

> thanks
> Lucas De Marchi
> 
> >                 /* Optional index missing, try the old one */
> >                 DBG(ctx, "lookup modules.builtin %s\n", alias);
> >                 err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);

//Peter
Lucas De Marchi May 11, 2021, 4:58 p.m. UTC | #3
On Mon, May 10, 2021 at 6:55 AM Peter Kjellerstedt
<peter.kjellerstedt@axis.com> wrote:
>
> > -----Original Message-----
> > From: Lucas De Marchi <lucas.de.marchi@gmail.com>
> > Sent: den 9 maj 2021 07:55
> > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > Cc: linux-modules <linux-modules@vger.kernel.org>; Peter Kjellerstedt
> > <peter.kjellerstedt@axis.com>
> > Subject: Re: [PATCH] libkmod: Always search modules.builtin if no alias
> > has been found
> >
> > On Tue, May 4, 2021 at 5:17 PM Peter Kjellerstedt <pkj@axis.com> wrote:
> > >
> > > Commit 89443220e broke the lookup for builtin modules. modules.builtin
> > > was no longer searched if kmod_lookup_alias_from_kernel_builtin_file()
> > > returned 0.
> > >
> > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > ---
> > >
> > > I do not know if this is the correct thing to do, or if the commit
> > > message makes any sense. However, it solves the problem we were seeing.
> > > We use fuse, which installs /etc/modules-load.d/fuse.conf to load the
> > > fuse kernel module. However, we have fuse built-in. Normally, the
> > > following can be seen in the log:
> > >
> > >   systemd-modules-load[192]: Module 'fuse' is built in
> > >
> > > but after commit 89443220e, we instead got:
> > >
> > >   systemd-modules-load[193]: Failed to find module 'fuse'
> > >
> > > //Peter
> > >
> > >  libkmod/libkmod-module.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> > > index 76a6dc3..6720930 100644
> > > --- a/libkmod/libkmod-module.c
> > > +++ b/libkmod/libkmod-module.c
> > > @@ -577,7 +577,7 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
> > >
> > >         DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
> > >         err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> > > -       if (err == -ENOSYS) {
> > > +       if (err == 0 || err == -ENOSYS) {
> >
> > So in your case you do have modules.builtin.modinfo, but fuse doesn't
> > show up there. On the other hand it is listed in modules.builtin.
> > Does modules.builtin.info contain anything or is it an empty file?
>
> We have neither modules.builtin.modinfo nor modules.builtin.info.
> A little googling turned out that modules.builtin.modinfo seems to have
> been introduced in 5.2, but this product uses a 4.19 based kernel.

ok, now I understood the entire context. So it seems the problem is
not that we are
missing the handling for return 0, but rather that
kmod_lookup_alias_from_kernel_builtin_file()
is not returning -ENOSYS when it should (index doesn't exist).  I
thought this was covered, but
obviously I was wrong. I will take a look what's going on.... we
should not handle err == 0 the same
way we handle err == -ENOSYS. If the index is missing we want to
fallback to the old one, but if
the index is there and we didn't find the module, we should just
return an error.

Lucas De Marchi

>
> > It seems to me something else is broken:  all modules in
> > modules.builtin should be in modules.builtin.modinfo as well. What is
> > the result of the following commands?
> >
> > grep fuse /lib/modules/$(uname -r)/modules.builtin
>
> kernel/fs/fuse/fuse.ko
>
> > grep fuse /lib/modules/$(uname -r)/modules.builtin.modinfo
>
> grep: /lib/modules/4.19.110-axis8/modules.builtin.modinfo: No such file or directory
>
> This is an embedded product built with our own distribution based on
> Poky Gatesgarth from the Yocto Project. The rootfs is read-only, including
> /lib/modules, so any contest there is created when the firmware image is
> built.
>
> > thanks
> > Lucas De Marchi
> >
> > >                 /* Optional index missing, try the old one */
> > >                 DBG(ctx, "lookup modules.builtin %s\n", alias);
> > >                 err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
>
> //Peter
>
Lucas De Marchi May 11, 2021, 6:01 p.m. UTC | #4
On Tue, May 11, 2021 at 9:58 AM Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
>
> On Mon, May 10, 2021 at 6:55 AM Peter Kjellerstedt
> <peter.kjellerstedt@axis.com> wrote:
> >
> > > -----Original Message-----
> > > From: Lucas De Marchi <lucas.de.marchi@gmail.com>
> > > Sent: den 9 maj 2021 07:55
> > > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > Cc: linux-modules <linux-modules@vger.kernel.org>; Peter Kjellerstedt
> > > <peter.kjellerstedt@axis.com>
> > > Subject: Re: [PATCH] libkmod: Always search modules.builtin if no alias
> > > has been found
> > >
> > > On Tue, May 4, 2021 at 5:17 PM Peter Kjellerstedt <pkj@axis.com> wrote:
> > > >
> > > > Commit 89443220e broke the lookup for builtin modules. modules.builtin
> > > > was no longer searched if kmod_lookup_alias_from_kernel_builtin_file()
> > > > returned 0.
> > > >
> > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > > ---
> > > >
> > > > I do not know if this is the correct thing to do, or if the commit
> > > > message makes any sense. However, it solves the problem we were seeing.
> > > > We use fuse, which installs /etc/modules-load.d/fuse.conf to load the
> > > > fuse kernel module. However, we have fuse built-in. Normally, the
> > > > following can be seen in the log:
> > > >
> > > >   systemd-modules-load[192]: Module 'fuse' is built in
> > > >
> > > > but after commit 89443220e, we instead got:
> > > >
> > > >   systemd-modules-load[193]: Failed to find module 'fuse'
> > > >
> > > > //Peter
> > > >
> > > >  libkmod/libkmod-module.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> > > > index 76a6dc3..6720930 100644
> > > > --- a/libkmod/libkmod-module.c
> > > > +++ b/libkmod/libkmod-module.c
> > > > @@ -577,7 +577,7 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
> > > >
> > > >         DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
> > > >         err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> > > > -       if (err == -ENOSYS) {
> > > > +       if (err == 0 || err == -ENOSYS) {
> > >
> > > So in your case you do have modules.builtin.modinfo, but fuse doesn't
> > > show up there. On the other hand it is listed in modules.builtin.
> > > Does modules.builtin.info contain anything or is it an empty file?
> >
> > We have neither modules.builtin.modinfo nor modules.builtin.info.
> > A little googling turned out that modules.builtin.modinfo seems to have
> > been introduced in 5.2, but this product uses a 4.19 based kernel.
>
> ok, now I understood the entire context. So it seems the problem is
> not that we are
> missing the handling for return 0, but rather that
> kmod_lookup_alias_from_kernel_builtin_file()
> is not returning -ENOSYS when it should (index doesn't exist).  I
> thought this was covered, but
> obviously I was wrong. I will take a look what's going on.... we
> should not handle err == 0 the same
> way we handle err == -ENOSYS. If the index is missing we want to
> fallback to the old one, but if
> the index is there and we didn't find the module, we should just
> return an error.

Ok, I think I see what's happening. If you have a recent kmod with an
old kernel,
you have this scenario:

1) modules.builtin.modinfo doesn't exist since kernel didn't create it
2) depmod will write and empty index to modules.builtin.alias.bin when
modules.builtin.modinfo
is not present

I have a fix to depmod to stop writing an empty index. I will submit
that for review.

thanks
Lucas De Marchi


>
> Lucas De Marchi
>
> >
> > > It seems to me something else is broken:  all modules in
> > > modules.builtin should be in modules.builtin.modinfo as well. What is
> > > the result of the following commands?
> > >
> > > grep fuse /lib/modules/$(uname -r)/modules.builtin
> >
> > kernel/fs/fuse/fuse.ko
> >
> > > grep fuse /lib/modules/$(uname -r)/modules.builtin.modinfo
> >
> > grep: /lib/modules/4.19.110-axis8/modules.builtin.modinfo: No such file or directory
> >
> > This is an embedded product built with our own distribution based on
> > Poky Gatesgarth from the Yocto Project. The rootfs is read-only, including
> > /lib/modules, so any contest there is created when the firmware image is
> > built.
> >
> > > thanks
> > > Lucas De Marchi
> > >
> > > >                 /* Optional index missing, try the old one */
> > > >                 DBG(ctx, "lookup modules.builtin %s\n", alias);
> > > >                 err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);
> >
> > //Peter
> >
Peter Kjellerstedt May 12, 2021, 1:30 p.m. UTC | #5
> -----Original Message-----
> From: Lucas De Marchi <lucas.de.marchi@gmail.com>
> Sent: den 11 maj 2021 20:02
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> Cc: linux-modules <linux-modules@vger.kernel.org>
> Subject: Re: [PATCH] libkmod: Always search modules.builtin if no alias
> has been found
> 
> On Tue, May 11, 2021 at 9:58 AM Lucas De Marchi
> <lucas.de.marchi@gmail.com> wrote:
> >
> > On Mon, May 10, 2021 at 6:55 AM Peter Kjellerstedt
> > <peter.kjellerstedt@axis.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Lucas De Marchi <lucas.de.marchi@gmail.com>
> > > > Sent: den 9 maj 2021 07:55
> > > > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > > Cc: linux-modules <linux-modules@vger.kernel.org>; Peter Kjellerstedt
> > > > <peter.kjellerstedt@axis.com>
> > > > Subject: Re: [PATCH] libkmod: Always search modules.builtin if no alias
> > > > has been found
> > > >
> > > > On Tue, May 4, 2021 at 5:17 PM Peter Kjellerstedt <pkj@axis.com> wrote:
> > > > >
> > > > > Commit 89443220e broke the lookup for builtin modules. modules.builtin
> > > > > was no longer searched if kmod_lookup_alias_from_kernel_builtin_file()
> > > > > returned 0.
> > > > >
> > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > > > ---
> > > > >
> > > > > I do not know if this is the correct thing to do, or if the commit
> > > > > message makes any sense. However, it solves the problem we were seeing.
> > > > > We use fuse, which installs /etc/modules-load.d/fuse.conf to load the
> > > > > fuse kernel module. However, we have fuse built-in. Normally, the
> > > > > following can be seen in the log:
> > > > >
> > > > >   systemd-modules-load[192]: Module 'fuse' is built in
> > > > >
> > > > > but after commit 89443220e, we instead got:
> > > > >
> > > > >   systemd-modules-load[193]: Failed to find module 'fuse'
> > > > >
> > > > > //Peter
> > > > >
> > > > >  libkmod/libkmod-module.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> > > > > index 76a6dc3..6720930 100644
> > > > > --- a/libkmod/libkmod-module.c
> > > > > +++ b/libkmod/libkmod-module.c
> > > > > @@ -577,7 +577,7 @@ KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
> > > > >
> > > > >         DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
> > > > >         err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
> > > > > -       if (err == -ENOSYS) {
> > > > > +       if (err == 0 || err == -ENOSYS) {
> > > >
> > > > So in your case you do have modules.builtin.modinfo, but fuse doesn't
> > > > show up there. On the other hand it is listed in modules.builtin.
> > > > Does modules.builtin.info contain anything or is it an empty file?
> > >
> > > We have neither modules.builtin.modinfo nor modules.builtin.info.
> > > A little googling turned out that modules.builtin.modinfo seems to have
> > > been introduced in 5.2, but this product uses a 4.19 based kernel.
> >
> > ok, now I understood the entire context. So it seems the problem is
> > not that we are missing the handling for return 0, but rather that
> > kmod_lookup_alias_from_kernel_builtin_file() is not returning -ENOSYS
> > when it should (index doesn't exist).  I thought this was covered, but
> > obviously I was wrong. I will take a look what's going on.... we
> > should not handle err == 0 the same way we handle err == -ENOSYS. If
> > the index is missing we want to fallback to the old one, but if the
> > index is there and we didn't find the module, we should just return
> > an error.
> 
> Ok, I think I see what's happening. If you have a recent kmod with an
> old kernel, you have this scenario:
> 
> 1) modules.builtin.modinfo doesn't exist since kernel didn't create it
> 2) depmod will write and empty index to modules.builtin.alias.bin when
> modules.builtin.modinfo is not present
> 
> I have a fix to depmod to stop writing an empty index. I will submit
> that for review.

I have tested the suggested solution and it solves the problem for us. 
Thank you.

//Peter

> thanks
> Lucas De Marchi
> 
> 
> >
> > Lucas De Marchi
> >
> > >
> > > > It seems to me something else is broken:  all modules in
> > > > modules.builtin should be in modules.builtin.modinfo as well. What
> is
> > > > the result of the following commands?
> > > >
> > > > grep fuse /lib/modules/$(uname -r)/modules.builtin
> > >
> > > kernel/fs/fuse/fuse.ko
> > >
> > > > grep fuse /lib/modules/$(uname -r)/modules.builtin.modinfo
> > >
> > > grep: /lib/modules/4.19.110-axis8/modules.builtin.modinfo: No such
> file or directory
> > >
> > > This is an embedded product built with our own distribution based on
> > > Poky Gatesgarth from the Yocto Project. The rootfs is read-only,
> including
> > > /lib/modules, so any contest there is created when the firmware image
> is
> > > built.
> > >
> > > > thanks
> > > > Lucas De Marchi
> > > >
> > > > >                 /* Optional index missing, try the old one */
> > > > >                 DBG(ctx, "lookup modules.builtin %s\n", alias);
> > > > >                 err = kmod_lookup_alias_from_builtin_file(ctx,
> alias, list);
> > >
> > > //Peter
> > >
diff mbox series

Patch

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 76a6dc3..6720930 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -577,7 +577,7 @@  KMOD_EXPORT int kmod_module_new_from_lookup(struct kmod_ctx *ctx,
 
 	DBG(ctx, "lookup modules.builtin.modinfo %s\n", alias);
 	err = kmod_lookup_alias_from_kernel_builtin_file(ctx, alias, list);
-	if (err == -ENOSYS) {
+	if (err == 0 || err == -ENOSYS) {
 		/* Optional index missing, try the old one */
 		DBG(ctx, "lookup modules.builtin %s\n", alias);
 		err = kmod_lookup_alias_from_builtin_file(ctx, alias, list);