Linux Kernel module notification bug
diff mbox series

Message ID 20210110175401.GB32505@pi3.com.pl
State New, archived
Headers show
Series
  • Linux Kernel module notification bug
Related show

Commit Message

Adam Zabrocki Jan. 10, 2021, 5:54 p.m. UTC
Hello,

I believe that the following commit does introduce a gentle "functionality 
bug":

"module: delay kobject uevent until after module init call":
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3

The official Linux Kernel API for the kernel module activities notification has
been divided based on the readiness 'stage' for such module. We have the
following stages:

        MODULE_STATE_LIVE,      /* Normal state. */
        MODULE_STATE_COMING,    /* Full formed, running module_init. */
        MODULE_STATE_GOING,     /* Going away. */
        MODULE_STATE_UNFORMED,  /* Still setting it up. */

LIVE means that the kernel module is correctly running and all initialization
work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.

In the described commit, creation of the KOBJECT has been moved after invoking
a notficiation of the newly formed kernel module state (LIVE). That's somehow
inconsistent from my understanding of the kernel modules notifiers logic.
Creation of the new objects (like KOBJ) should be done before notification of
the stage LIVE is invoked.

This commit breaks some of the projects that rely on the LIVE notification to
monitor when the newly loaded module is ready.

I believe that the latest time when 'kobject_uevent' function can be called is
just before invoking 'blocking_notifier_call_chain', e.g:


Thanks,
Adam

--
pi3 (pi3ki31ny) - pi3 (at) itsec pl
http://pi3.com.pl

Comments

Jessica Yu Jan. 11, 2021, 2:20 p.m. UTC | #1
+++ Adam Zabrocki [10/01/21 18:54 +0100]:
>Hello,
>
>I believe that the following commit does introduce a gentle "functionality
>bug":
>
>"module: delay kobject uevent until after module init call":
>https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
>
>The official Linux Kernel API for the kernel module activities notification has
>been divided based on the readiness 'stage' for such module. We have the
>following stages:
>
>        MODULE_STATE_LIVE,      /* Normal state. */
>        MODULE_STATE_COMING,    /* Full formed, running module_init. */
>        MODULE_STATE_GOING,     /* Going away. */
>        MODULE_STATE_UNFORMED,  /* Still setting it up. */
>
>LIVE means that the kernel module is correctly running and all initialization
>work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
>
>In the described commit, creation of the KOBJECT has been moved after invoking
>a notficiation of the newly formed kernel module state (LIVE). That's somehow
>inconsistent from my understanding of the kernel modules notifiers logic.
>Creation of the new objects (like KOBJ) should be done before notification of
>the stage LIVE is invoked.

I'm confused. We're not creating any kobjects here. That is all done
in mod_sysfs_setup(), which is called while the module is still
COMING.  What that commit does is delay telling userspace about the
module (specifically, systemd/udev) until the module is basically
ready. Systemd was basically receiving the uevent too early, before
the module has initialized, hence we decided to delay the uevent [1].

>This commit breaks some of the projects that rely on the LIVE notification to
>monitor when the newly loaded module is ready.

Hm, could you please explain specifically what is the issue you're seeing?
What projects is it breaking?

Thanks,

Jessica

[1] https://github.com/systemd/systemd/issues/17586
Adam Zabrocki Jan. 12, 2021, 12:15 a.m. UTC | #2
Hello,

On Mon, Jan 11, 2021 at 03:20:48PM +0100, Jessica Yu wrote:
> +++ Adam Zabrocki [10/01/21 18:54 +0100]:
> > Hello,
> > 
> > I believe that the following commit does introduce a gentle "functionality
> > bug":
> > 
> > "module: delay kobject uevent until after module init call":
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
> > 
> > The official Linux Kernel API for the kernel module activities notification has
> > been divided based on the readiness 'stage' for such module. We have the
> > following stages:
> > 
> >        MODULE_STATE_LIVE,      /* Normal state. */
> >        MODULE_STATE_COMING,    /* Full formed, running module_init. */
> >        MODULE_STATE_GOING,     /* Going away. */
> >        MODULE_STATE_UNFORMED,  /* Still setting it up. */
> > 
> > LIVE means that the kernel module is correctly running and all initialization
> > work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
> > 
> > In the described commit, creation of the KOBJECT has been moved after invoking
> > a notficiation of the newly formed kernel module state (LIVE). That's somehow
> > inconsistent from my understanding of the kernel modules notifiers logic.
> > Creation of the new objects (like KOBJ) should be done before notification of
> > the stage LIVE is invoked.
> 
> I'm confused. We're not creating any kobjects here. That is all done
> in mod_sysfs_setup(), which is called while the module is still
> COMING.  What that commit does is delay telling userspace about the
> module (specifically, systemd/udev) until the module is basically
> ready. Systemd was basically receiving the uevent too early, before
> the module has initialized, hence we decided to delay the uevent [1].
> 

Sorry for the confusion on my side. I was referring to the internal state of 
the KOBJ itself which is being actively modified when uevent is sent. During 
the module creation KOBJ_ADD modifies 'kobj->state_add_uevent_sent'. Until 
recent commit, kernel didn't modify KOBJ after sending LIVE notification.

> > This commit breaks some of the projects that rely on the LIVE notification to
> > monitor when the newly loaded module is ready.
> 
> Hm, could you please explain specifically what is the issue you're seeing?
> What projects is it breaking?
> 

I'm specifically referencing these projects which are tracking kernel modules
for integrity purpose (e.g. anti-rootkit tools) like Linux Kernel Runtime 
Guard.
It is possible to modify these tools to adopt and take into account 
modification of KOBJ after LIVE state notification. However, from my 
understanding of the kernel modules notifiers logic, KOBJ should be fully 
formed at this stage.

Thanks,
Adam

> Thanks,
> 
> Jessica
> 
> [1] https://github.com/systemd/systemd/issues/17586
Jessica Yu Jan. 12, 2021, 10:46 a.m. UTC | #3
+++ Adam Zabrocki [12/01/21 01:15 +0100]:
>Hello,
>
>On Mon, Jan 11, 2021 at 03:20:48PM +0100, Jessica Yu wrote:
>> +++ Adam Zabrocki [10/01/21 18:54 +0100]:
>> > Hello,
>> >
>> > I believe that the following commit does introduce a gentle "functionality
>> > bug":
>> >
>> > "module: delay kobject uevent until after module init call":
>> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
>> >
>> > The official Linux Kernel API for the kernel module activities notification has
>> > been divided based on the readiness 'stage' for such module. We have the
>> > following stages:
>> >
>> >        MODULE_STATE_LIVE,      /* Normal state. */
>> >        MODULE_STATE_COMING,    /* Full formed, running module_init. */
>> >        MODULE_STATE_GOING,     /* Going away. */
>> >        MODULE_STATE_UNFORMED,  /* Still setting it up. */
>> >
>> > LIVE means that the kernel module is correctly running and all initialization
>> > work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
>> >
>> > In the described commit, creation of the KOBJECT has been moved after invoking
>> > a notficiation of the newly formed kernel module state (LIVE). That's somehow
>> > inconsistent from my understanding of the kernel modules notifiers logic.
>> > Creation of the new objects (like KOBJ) should be done before notification of
>> > the stage LIVE is invoked.
>>
>> I'm confused. We're not creating any kobjects here. That is all done
>> in mod_sysfs_setup(), which is called while the module is still
>> COMING.  What that commit does is delay telling userspace about the
>> module (specifically, systemd/udev) until the module is basically
>> ready. Systemd was basically receiving the uevent too early, before
>> the module has initialized, hence we decided to delay the uevent [1].
>>
>
>Sorry for the confusion on my side. I was referring to the internal state of
>the KOBJ itself which is being actively modified when uevent is sent. During
>the module creation KOBJ_ADD modifies 'kobj->state_add_uevent_sent'. Until
>recent commit, kernel didn't modify KOBJ after sending LIVE notification.
>
>> > This commit breaks some of the projects that rely on the LIVE notification to
>> > monitor when the newly loaded module is ready.
>>
>> Hm, could you please explain specifically what is the issue you're seeing?
>> What projects is it breaking?
>>
>
>I'm specifically referencing these projects which are tracking kernel modules
>for integrity purpose (e.g. anti-rootkit tools) like Linux Kernel Runtime
>Guard.
>It is possible to modify these tools to adopt and take into account
>modification of KOBJ after LIVE state notification. However, from my
>understanding of the kernel modules notifiers logic, KOBJ should be fully
>formed at this stage.

Ah I see, thanks for the clarification. I was under the impression
that kobj->state_add_uevent_sent is an internal field interesting
only to lib/kobject.c, and did not know tools like you mention are
implicitly tracking that.

In any case, I think your suggested change doesn't affect the
systemd/udev issue we were trying to fix, as long as the uevent is
sent after module init(). Would you mind sending a patch? (with a
Fixes: tag and including the explanations you've provided in this
thread in the changelog)

Thanks!

Jessica
Greg KH Jan. 12, 2021, 11 a.m. UTC | #4
On Tue, Jan 12, 2021 at 11:46:55AM +0100, Jessica Yu wrote:
> +++ Adam Zabrocki [12/01/21 01:15 +0100]:
> > Hello,
> > 
> > On Mon, Jan 11, 2021 at 03:20:48PM +0100, Jessica Yu wrote:
> > > +++ Adam Zabrocki [10/01/21 18:54 +0100]:
> > > > Hello,
> > > >
> > > > I believe that the following commit does introduce a gentle "functionality
> > > > bug":
> > > >
> > > > "module: delay kobject uevent until after module init call":
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
> > > >
> > > > The official Linux Kernel API for the kernel module activities notification has
> > > > been divided based on the readiness 'stage' for such module. We have the
> > > > following stages:
> > > >
> > > >        MODULE_STATE_LIVE,      /* Normal state. */
> > > >        MODULE_STATE_COMING,    /* Full formed, running module_init. */
> > > >        MODULE_STATE_GOING,     /* Going away. */
> > > >        MODULE_STATE_UNFORMED,  /* Still setting it up. */
> > > >
> > > > LIVE means that the kernel module is correctly running and all initialization
> > > > work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
> > > >
> > > > In the described commit, creation of the KOBJECT has been moved after invoking
> > > > a notficiation of the newly formed kernel module state (LIVE). That's somehow
> > > > inconsistent from my understanding of the kernel modules notifiers logic.
> > > > Creation of the new objects (like KOBJ) should be done before notification of
> > > > the stage LIVE is invoked.
> > > 
> > > I'm confused. We're not creating any kobjects here. That is all done
> > > in mod_sysfs_setup(), which is called while the module is still
> > > COMING.  What that commit does is delay telling userspace about the
> > > module (specifically, systemd/udev) until the module is basically
> > > ready. Systemd was basically receiving the uevent too early, before
> > > the module has initialized, hence we decided to delay the uevent [1].
> > > 
> > 
> > Sorry for the confusion on my side. I was referring to the internal state of
> > the KOBJ itself which is being actively modified when uevent is sent. During
> > the module creation KOBJ_ADD modifies 'kobj->state_add_uevent_sent'. Until
> > recent commit, kernel didn't modify KOBJ after sending LIVE notification.
> > 
> > > > This commit breaks some of the projects that rely on the LIVE notification to
> > > > monitor when the newly loaded module is ready.
> > > 
> > > Hm, could you please explain specifically what is the issue you're seeing?
> > > What projects is it breaking?
> > > 
> > 
> > I'm specifically referencing these projects which are tracking kernel modules
> > for integrity purpose (e.g. anti-rootkit tools) like Linux Kernel Runtime
> > Guard.
> > It is possible to modify these tools to adopt and take into account
> > modification of KOBJ after LIVE state notification. However, from my
> > understanding of the kernel modules notifiers logic, KOBJ should be fully
> > formed at this stage.
> 
> Ah I see, thanks for the clarification. I was under the impression
> that kobj->state_add_uevent_sent is an internal field interesting
> only to lib/kobject.c, and did not know tools like you mention are
> implicitly tracking that.

There is no in-kernel tools/users tracking stuff like this, so this is
not an issue.  The internals of kobjects can change at any time, there
is no "stable api" here at all.

thanks,

greg k-h

Patch
diff mbox series

diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..7d56b1b07237 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3681,14 +3681,14 @@  static noinline int do_init_module(struct module *mod)
                dump_stack();
        }

+       /* Delay uevent until module has finished its init routine */
+       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
+
        /* Now it's a first class citizen! */
        mod->state = MODULE_STATE_LIVE;
        blocking_notifier_call_chain(&module_notify_list,
                                     MODULE_STATE_LIVE, mod);

-       /* Delay uevent until module has finished its init routine */
-       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
-
        /*
         * We need to finish all async code before the module init sequence
         * is done.  This has potential to deadlock.  For example, a newly