Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 8 Oct 2019 15:35:34 -0400
From:   Joe Lawrence <joe.lawrence@redhat.com>
To:     Miroslav Benes <mbenes@suse.cz>
Cc:     rostedt@goodmis.org, mingo@redhat.com, jpoimboe@redhat.com,
        jikos@kernel.org, pmladek@suse.com, linux-kernel@vger.kernel.org,
        live-patching@vger.kernel.org
Subject: Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag
Message-ID: <20191008193534.GA16675@redhat.com>
References: <20191007081714.20259-1-mbenes@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191007081714.20259-1-mbenes@suse.cz>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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