kmod/core: account for failing modules in notifier

[joe-lawrence]

The module notifier currently only handles newly loaded modules in the
MODULE_STATE_COMING state. If target modules need to be unloaded, the
any kpatch module that patches it must first be disabled, releasing
module references held against the target module. When the kpatch
modules are disabled, the target module is unpatched and the kpatch
core's data structures updated accordingly.

If a loading module happens to fail its init routine (missing hardware
for example), that module will not complete loading. The kpatch core
doesn't properly account for this "phantom" target module, so when the
kpatch patch module is removed, it spews out an ugly warning when
attempting to remove a non-existing ftrace filter on the target module.

In the problematic case above, the module handler is notified of the
failing module in its MODULE_STATE_GOING state. Add code to the handler
to clear any ftrace filters, call pre and post unpatch callbacks, etc.

Fixes #699.

Signed-off-by: Joe Lawrence joe.lawrence@redhat.com

[joe-lawrence]

test3.patch.txt
module-probe-failed-testing.txt

Testing includes a case in which the aesni_intel driver fails to load, but a loaded kpatch module has pre/post (un)patch callbacks defined against it. The kpatch core doesn't properly account for the target module failing its init, and when unloading the kpatch module, it can crash the machine. See the attached module-probe-failed-testing.txt for before/after details.

[joe-lawrence]

To be perfectly symmetric, I wanted to call kpatch_link_object() here, but that fails with "can't remove ftrace filter at address". Am I missing something? Since the module isn't going to be loaded anyway, clearing the mod pointer was good enough to trip up any future kpatch_object_linked() checks.

[jpoimboe]

Yeah, a kpatch_unlink_object() would be better here. I'm not sure why it gives that error. Maybe the module init error path unregisters all the ftrace stuff before calling the going notifier? Or maybe ftrace has a going notifier which was called first?

[joe-lawrence]

Hmm, if I move this teardown code up into the module coming case (right after it runs the post-patch callback), it all runs w/o incident. I suspect you're right that something external to kpatch is getting in the way here. I'm looking through the load_module() and do_init_module() code now, though nothing is jumping out yet.

[joe-lawrence]

Bingo, here it is: ftrace_module_notify_exit() -> ftrace_release_mod(mod) (And renamed in newer RHEL7 kernels as ftrace_module_notify(). Looking that the module_notify_list in crash, I can see that it's before the kpatch module notifier.

[jpoimboe]

Ah, then we may end up having to register two notifiers: the coming notifier with priority INT_MIN so it's called last, and the going notifier with priority INT_MAX, so it's called first.

[jpoimboe]

The location of the 'coming_out' label is a bit awkward IMO. In general the code might work out to be a little cleaner if there were a helper for MODULE_STATE_GOING, like

        if (action == MODULE_STATE_GOING) {
                kpatch_module_notify_going();
                goto out;
        }

        /* handle coming state here */

[jpoimboe]

Yeah, a kpatch_unlink_object() would be better here. I'm not sure why it gives that error. Maybe the module init error path unregisters all the ftrace stuff before calling the going notifier? Or maybe ftrace has a going notifier which was called first?

[joe-lawrence]

v2:

• clear object->mod in kpatch_unlink_object() so that future kpatch_object_linked() checks will fail
• split into separate coming and going notifiers so we can register last and first (respectively) on the notifier list, h/t Josh for the suggestion

[jpoimboe]

I think we need to unregister the coming notifier here on error.

[joe-lawrence]

v3:

• If the kpatch_module_nb_going handler fails to register, be sure to unregister the kpatch_module_nb_coming handler in the kpatch_init() error path.