[BUG] Incorrect ordering of klp and ftrace module notifier callbacks

[BUG] Incorrect ordering of klp and ftrace module notifier callbacks

[Jessica Yu]

Hi all,

I recently found that livepatch is broken for patches to modules on the latest
for-next branch [1]. In its current state, livepatch is able to patch a module
if the module is *already* loaded, but will fail if the target module loads
after the patch module loads. The patched, new functions are not getting
called, and the old functions are running instead.

I ran a git bisect with the last good commit as -
[b56b36e] livepatch: Cleanup module page permission changes
and the first bad commit identified was -
[5156dca] ftrace: Fix the race between ftrace and insmod

In short, that commit moves the call to ftrace_module_enable() out of
ftrace_module_init() to the ftrace_module_notify() callback, and is called when
the module state is COMING. The problem is, livepatch also has a module
notifier callback that enables a patch for COMING modules, but makes the
assumption that ftrace_module_enable() has already been called. So ordering is
important for livepatch to get this right, but with the current notifier
priorities, we get the ordering wrong.

So we currently get the following callback ordering when a target module loads:
1) klp_module_notify() [priority: INT_MIN+1, so this runs first] -
Sees target module is COMING, tries to enable the corresponding patch
(but ftrace_module_enable() hasn't been called yet)
2) ftrace_module_notify() [priority: INT_MIN, runs last] -
Sees module is COMING, calls ftrace_module_enable(), but it's too late for
livepatch

And the following ordering when the target module unloads:
1) klp_module_notify() [priority: INT_MIN+1] -
Sees target module GOING, disables patch
2) ftrace_module_notify [priority: INT_MIN] -
Sees module GOING, calls ftrace_release_mod()

The unload ordering is correct, but for the load case the calls need to be
flipped around (ftrace first).

So it doesn't look like a simple priority tweak will work, since we want a
ftrace->klp->klp->ftrace ordering from module load->module unload. Perhaps we
can split the ftrace and klp notifiers into separate coming and going handlers
with the appropriate priorities set to enforce correct ordering (not terribly
pretty, but it works). Is there a better solution?

[1] git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-next

Thanks,
Jessica

Re: [BUG] Incorrect ordering of klp and ftrace module notifier callbacks

[Jiri Kosina]

On Thu, 28 Jan 2016, Jessica Yu wrote:

> So it doesn't look like a simple priority tweak will work, since we want a
> ftrace->klp->klp->ftrace ordering from module load->module unload. Perhaps we
> can split the ftrace and klp notifiers into separate coming and going handlers
> with the appropriate priorities set to enforce correct ordering (not terribly
> pretty, but it works). Is there a better solution?

Given the facts that:

- we want the ordering ftrace -> klp for load and klp -> ftrace for 
  unload

- we already do have separate klp_module_notify_coming() and 
  klp_module_notify_going(), and klp_module_notify() is just a stub that 
  decides which one to call

I'd suggest we keep just a single ftrace_module_notify() (with INT_MIN+1, 
priority), and then introduce separate notifiers

- klp_module_notify_going() with INT_MIN+2, that triggers action iff 
  action == MODULE_STATE_GOING

- klp_module_notify_going() with INT_MIN, that triggers action only iff 
  action == MODULE_STATE_COMING

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [BUG] Incorrect ordering of klp and ftrace module notifier callbacks

[Jiri Kosina]

On Thu, 28 Jan 2016, Jiri Kosina wrote:

> - klp_module_notify_going() with INT_MIN+2, that triggers action iff 
>   action == MODULE_STATE_GOING
> 
> - klp_module_notify_going() with INT_MIN, that triggers action only iff 
>   action == MODULE_STATE_COMING

I *of course* created the mismatch between the notifier name and the 
action value above completely deliberately to excercise the attention of 
the readers :p

Sorry for the noise,

-- 
Jiri Kosina
SUSE Labs