[Bug runtime/16861] New: probes aren't re-registered after module reload

[Bug runtime/16861] New: probes aren't re-registered after module reload

[jistone at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16861

            Bug ID: 16861
           Summary: probes aren't re-registered after module reload
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: runtime
          Assignee: systemtap at sourceware dot org
          Reporter: jistone at redhat dot com

With emit_module_refresh, we can support probes in modules that weren't loaded
at the time stap started.  However, if a module is unloaded and then loaded
again, we miss the chance to register the probes anew.

For example, on a system not already using btrfs.ko:

$ stap -e 'probe module("btrfs").function("*_btrfs_fs").call {
println(ppfunc()) }' -c 'sudo sh -c "modprobe btrfs; modprobe -r btrfs;
modprobe btrfs; modprobe -r btrfs"'
init_btrfs_fs
exit_btrfs_fs

The init and exit are only noticed once, despite being loaded twice.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug runtime/16861] probes aren't re-registered after module reload

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16861

Jonathan Lebon <jlebon at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jlebon at redhat dot com

--- Comment #1 from Jonathan Lebon <jlebon at redhat dot com> ---
OK, I've looked into this a bit more. It seems like we do register and
unregister when it "makes sense", i.e.

- Upon MODULE_STATE_COMING, we update all the addresses, and then register all
kprobes.
- Upon MODULE_STATE_LIVE, we zero out any init section, and then unregister any
init kprobes.
- Upon MODULE_STATE_GOING, we zero out all sections, and then unregister all
kprobes.

However, it looks like kprobes doesn't like it when we unregister things during
the MODULE_STATE_LIVE and MODULE_STATE_GOING events. This may be due to the
fact that kprobes already checks for these events as well (and is notified
before us) and accordingly disarms no longer valid kprobes (see
kprobes_module_callback()). For that reason, unregistration for no longer valid
module addresses is not necessary.

If we instead unregister and re-register during the MODULE_STATE_COMING event,
then everything works as expected. Note by the way that this is the approach
also done in trace_kprobe.c (see trace_kprobe_module_callback()).

Tentative patch to follow.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug runtime/16861] probes aren't re-registered after module reload

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16861

--- Comment #2 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 7647
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7647&action=edit
Patch

In this patch, we only call back to systemtap_module_refresh() for the COMING
event. We also pass the module name so that only kprobes for that module are
refreshed.

With the patch:

$ stap -e 'probe module("btrfs").function("*_btrfs_fs").call { print
ln(ppfunc()) }' -c 'sudo sh -c "modprobe btrfs; modprobe -r btrfs; modprobe
btrfs; modprobe -r btrfs"'
init_btrfs_fs
exit_btrfs_fs
init_btrfs_fs
exit_btrfs_fs
$

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug runtime/16861] probes aren't re-registered after module reload

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16861

Jonathan Lebon <jlebon at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7647|0                           |1
        is obsolete|                            |

--- Comment #3 from Jonathan Lebon <jlebon at redhat dot com> ---
Created attachment 7648
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7648&action=edit
Patch

Sorry, previous patch was the wrong one. :)

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug runtime/16861] probes aren't re-registered after module reload

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16861

--- Comment #4 from Jonathan Lebon <jlebon at redhat dot com> ---
OK, I've decided to *really* look into this and see why kprobes seems to be
misbehaving. The answer is that we are the ones actually misbehaving. :)

The key is that we are actually being notified *before* the kprobes callback,
not after (I believe the comments in transport.c which is based on
kernel/trace/trace_kprobe.c, are wrong -- notifiers are called from high to low
priority number, not the other way around, see notifier_chain_register()).

So what would happen is that (prior to the patch I posted) we would call
unregister_kprobe() during the GOING event, which would set the DISABLED flag
as part of the process. That DISABLED flag stayed in the struct and is what
caused the behaviour in the PR in the first place (i.e. the next register()
looked like it didn't work, when really it did register but just remained
disabled).

On the other hand, with the patch I posted above, we would call
unregister_kprobe() only at the next COMING event, at which point it doesn't
bother to set the DISABLED flag because kprobes had already flagged it as GONE
during the previous GOING event (see the kprobe_disabled() predicate, which
looks at both the DISABLED and the GONE flags). This is why the following
register() call worked.

So really all we needed to do was change the priority of the notifier to be
less than that of the kprobes callback and of course make sure that the flags
member is cleared (or just zero out the whole struct).

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug runtime/16861] probes aren't re-registered after module reload

[jlebon at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16861

Jonathan Lebon <jlebon at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Jonathan Lebon <jlebon at redhat dot com> ---
The fix for this is part of the jlebon/onthefly branch (commit 19d62b5), which
has now been merged into master (merge commit 2e9f9de).

-- 
You are receiving this mail because:
You are the assignee for the bug.