lkml.org 
[lkml]   [2019]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag
    On Mon, Oct 07, 2019 at 10:17:11AM +0200, Miroslav Benes wrote:
    > Livepatch uses ftrace for redirection to new patched functions. It is
    > thus directly affected by ftrace sysctl knobs such as ftrace_enabled.
    > Setting ftrace_enabled to 0 also disables all live patched functions. It
    > is not a problem per se, because only administrator can set sysctl
    > values, but it still may be surprising.
    >
    > Introduce PERMANENT ftrace_ops flag to amend this. If the
    > FTRACE_OPS_FL_PERMANENT is set, the tracing of the function is not
    > disabled. Such ftrace_ops can still be unregistered in a standard way.
    >
    > The patch set passes ftrace and livepatch kselftests.
    >
    > Miroslav Benes (3):
    > ftrace: Make test_rec_ops_needs_regs() generic
    > ftrace: Introduce PERMANENT ftrace_ops flag
    > livepatch: Use FTRACE_OPS_FL_PERMANENT
    >
    > Documentation/trace/ftrace-uses.rst | 6 ++++
    > Documentation/trace/ftrace.rst | 2 ++
    > include/linux/ftrace.h | 8 +++--
    > kernel/livepatch/patch.c | 3 +-
    > kernel/trace/ftrace.c | 47 ++++++++++++++++++++++++-----
    > 5 files changed, 55 insertions(+), 11 deletions(-)
    >
    > --
    > 2.23.0
    >

    Hi Miroslav,

    I wonder if the opposite would be more intuitive: when ftrace_enabled is
    not set, don't allow livepatches to register ftrace filters and
    likewise, don't allow ftrace_enabled to be unset if any livepatches are
    already registered. I guess you could make an argument either way, but
    just offering another option. Perhaps livepatches should follow similar
    behavior of other ftrace clients (like perf probes?)

    As for the approach in this patchset, is it consistent that livepatches
    loaded after setting ftrace_enabled to 0 will successfully load, but not
    execute their new code... but then when ftrace_enabled is toggled, the
    new livepatch code remains on?

    For example:

    1 - Turn ftrace_enabled off and load the /proc/cmdline livepatch test
    case, note that it reports a success patching transition, but
    doesn't run new its code:

    % dmesg -C
    % sysctl kernel.ftrace_enabled=0
    kernel.ftrace_enabled = 0
    % insmod lib/livepatch/test_klp_livepatch.ko
    % echo $?
    0
    % dmesg
    [ 450.579980] livepatch: enabling patch 'test_klp_livepatch'
    [ 450.581243] livepatch: 'test_klp_livepatch': starting patching transition
    [ 451.942971] livepatch: 'test_klp_livepatch': patching complete
    % cat /proc/cmdline
    BOOT_IMAGE=(hd0,msdos1)/boot/vmlinuz-5.4.0-rc2+ root=UUID=c42bb089-b5c1-4e17-82bd-132f55bee54c ro console=ttyS0 console=ttyS0,115200n8 no_timer_check net.ifnames=0 crashkernel=auto

    2 - Turn ftrace_enabled on and see that the livepatch now works:

    % sysctl kernel.ftrace_enabled=1
    kernel.ftrace_enabled = 1
    % cat /proc/cmdline
    test_klp_livepatch: this has been live patched

    3 - Turn ftrace_enabled off and see that it's still enabled:

    % sysctl kernel.ftrace_enabled=0
    kernel.ftrace_enabled = 0
    % cat /proc/cmdline
    test_klp_livepatch: this has been live patched

    Steps 2 and 3 match the behavior described by the patchset, but I was
    particularly wondering what you thought about step 1.

    IMHO, I would expect step 1 to fully enable the livepatch, or at the
    very least, not report a patch transition (though that may confuse
    userspace tools waiting for that report).

    Thanks,

    -- Joe

    \
     
     \ /
      Last update: 2019-10-08 21:36    [W:3.234 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site