ftrace: host klp and bpf trampoline together

PATCH Superseded Song Liu 5 participants 17 messages Jul 18, 2022 – Jul 19, 2022

Subsystems (7)

x86 BPF Tracing BPF core +3 more subsystems

lore

Share & tools

lore

Song Liu

author maintainer

2022-07-18 05:55 UTC #0 lore raw

v4 cover ftrace: host klp and bpf trampoline together

Changes v3 => v4:
1. Fix build errors for different config. (kernel test robot)

Changes v2 => v3:
1. Major rewrite after discussions with Steven Rostedt. [1]
2. Remove SHARE_IPMODIFY flag from ftrace code. Instead use the callback function to communicate this information. (Steven)
3. Add cleanup_direct_functions_after_ipmodify() to clear SHARE_IPMODIFY on the DIRECT ops when the IPMODIFY ops is removed.

Changes v1 => v2:
1. Fix build errors for different config. (kernel test robot)

Kernel Live Patch (livepatch, or klp) and bpf trampoline are important features for modern systems. This set allows the two to work on the same kernel function as the same time.

live patch uses ftrace with IPMODIFY, while bpf trampoline use direct ftrace. Existing policy does not allow the two to attach to the same kernel function. This is changed by fine tuning ftrace IPMODIFY policy, and allows one IPMODIFY ftrace_ops and one DIRECT ftrace_ops on the same kernel function at the same time. Please see patch 2 and 4 for more details.

Note that, one of the constraint here is to let bpf trampoline use direct call when it is not working on the same function as live patch. This is achieved by allowing ftrace code to ask bpf trampoline to make changes.

[1] https://lore.kernel.org/all/[email protected]/

Jiri Olsa (1):
bpf, x64: Allow to use caller address from stack

Song Liu (3):
ftrace: add modify_ftrace_direct_multi_nolock
ftrace: allow IPMODIFY and DIRECT ops on the same function
bpf: support bpf_trampoline on functions with IPMODIFY (e.g.
livepatch)

 arch/x86/net/bpf_jit_comp.c |  13 +-
 include/linux/bpf.h         |  13 ++
 include/linux/ftrace.h      |  43 +++++
 kernel/bpf/trampoline.c     | 158 +++++++++++++++++--
 kernel/trace/ftrace.c       | 306 +++++++++++++++++++++++++++++++-----
 5 files changed, 473 insertions(+), 60 deletions(-)

--
2.30.2

Song Liu

author maintainer

2022-07-18 05:55 UTC #1 lore raw

PATCH v4 1/4 ftrace: add modify_ftrace_direct_multi_nolock

This is similar to modify_ftrace_direct_multi, but does not acquire direct_mutex. This is useful when direct_mutex is already locked by the user.

Signed-off-by Song Liu <[email protected]>

---
 include/linux/ftrace.h |  5 +++
 kernel/trace/ftrace.c  | 85 ++++++++++++++++++++++++++++++------------
 2 files changed, 67 insertions(+), 23 deletions(-)

include/linux/ftrace.h +5 -0

index 979f6bfa2c25..acb35243ce5d 100644
@@ -340,6 +340,7 @@ unsigned long ftrace_find_rec_direct(unsigned long ip);
 int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
 int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
 int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
+int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr);
 
 #else
 struct ftrace_ops;

@@ -384,6 +385,10 @@ static inline int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned lo
 {
 	return -ENODEV;
 }
+static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS

kernel/trace/ftrace.c +62 ~~-23~~

110 lines not on this page · raw message

Signature · 1 line

2.30.2

Steven Rostedt

maintainer

2022-07-19 17:57 UTC #2 lore raw

Re: [PATCH v4 bpf-next 1/4] ftrace: add modify_ftrace_direct_multi_nolock

Quoted 1 line · On Sun, 17 Jul 2022 22:54:46 -0700 Song Liu <[email protected]> wrote:

Subject: [PATCH v4 bpf-next 1/4] ftrace: add modify_ftrace_direct_multi_nolock

FYI, ftrace and tracing style is to capitalize the first word in the subject.

ftrace: Add modify_ftrace_direct_multi_nolock

Please fix this in the next version.

-- Steve

Quoted 8 lines

Date: Sun, 17 Jul 2022 22:54:46 -0700
Message-ID: <[email protected]>
This is similar to modify_ftrace_direct_multi, but does not acquire direct_mutex. This is useful when direct_mutex is already locked by the user.
Signed-off-by Song Liu <[email protected]>

Song Liu

author maintainer

2022-07-18 05:55 UTC #3 lore raw

PATCH v4 2/4 ftrace: allow IPMODIFY and DIRECT ops on the same function

In reply to Song Liu

IPMODIFY (livepatch) and DIRECT (bpf trampoline) ops are both important users of ftrace. It is necessary to allow them work on the same function at the same time.

First, DIRECT ops no longer specify IPMODIFY flag. Instead, DIRECT flag is handled together with IPMODIFY flag in __ftrace_hash_update_ipmodify().

The, a callback function, ops_func, is added to ftrace_ops. This is used by ftrace core code to understand whether the DIRECT ops can share with an IPMODIFY ops. To share with IPMODIFY ops, the DIRECT ops need to implement the callback function and adjust the direct trampoline accordingly.

If DIRECT ops is attached before the IPMODIFY ops, ftrace core code calls ENABLE_SHARE_IPMODIFY_PEER on the DIRECT ops before registering the IPMODIFY ops.

If IPMODIFY ops is attached before the DIRECT ops, ftrace core code calls ENABLE_SHARE_IPMODIFY_SELF in __ftrace_hash_update_ipmodify. Owner of the DIRECT ops may return 0 if the DIRECT trampoline can share with IPMODIFY, so error code otherwise. The error code is propagated to register_ftrace_direct_multi so that onwer of the DIRECT trampoline can handle it properly.

For more details, please refer to comment before enum ftrace_ops_cmd.

Link https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Song Liu <[email protected]>

---
 include/linux/ftrace.h |  38 +++++++
 kernel/trace/ftrace.c  | 221 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 243 insertions(+), 16 deletions(-)

include/linux/ftrace.h +38 -0

index acb35243ce5d..306bf08acda6 100644
@@ -208,6 +208,43 @@ enum {
 	FTRACE_OPS_FL_DIRECT			= BIT(17),
 };
 
+/*
+ * FTRACE_OPS_CMD_* commands allow the ftrace core logic to request changes
+ * to a ftrace_ops. Note, the requests may fail.
+ *
+ * ENABLE_SHARE_IPMODIFY_SELF - enable a DIRECT ops to work on the same
+ *                              function as an ops with IPMODIFY. Called
+ *                              when the DIRECT ops is being registered.
+ *                              This is called with both direct_mutex and
+ *                              ftrace_lock are locked.
+ *
+ * ENABLE_SHARE_IPMODIFY_PEER - enable a DIRECT ops to work on the same
+ *                              function as an ops with IPMODIFY. Called
+ *                              when the other ops (the one with IPMODIFY)
+ *                              is being registered.
+ *                              This is called with direct_mutex locked.
+ *
+ * DISABLE_SHARE_IPMODIFY_PEER - disable a DIRECT ops to work on the same
+ *                               function as an ops with IPMODIFY. Called
+ *                               when the other ops (the one with IPMODIFY)
+ *                               is being unregistered.
+ *                               This is called with direct_mutex locked.
+ */
+enum ftrace_ops_cmd {
+	FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_SELF,
+	FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_PEER,
+	FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY_PEER,
+};
+
+/*
+ * For most ftrace_ops_cmd,
+ * Returns:
+ *        0 - Success.
+ *        -EBUSY - The operation cannot process
+ *        -EAGAIN - The operation cannot process tempoorarily.
+ */
+typedef int (*ftrace_ops_func_t)(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 /* The hash used to know what functions callbacks trace */
 struct ftrace_ops_hash {

@@ -250,6 +287,7 @@ struct ftrace_ops {
 	unsigned long			trampoline;
 	unsigned long			trampoline_size;
 	struct list_head		list;
+	ftrace_ops_func_t		ops_func;
 #endif
 };

kernel/trace/ftrace.c +205 ~~-16~~

320 lines not on this page · raw message

Signature · 1 line

2.30.2

kernel test robot

build bot

2022-07-18 07:43 UTC #4 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

I love your patch! Perhaps something to improve:

Hi Song,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220718/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
        git checkout 9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

kernel/trace/ftrace.c: In function 'register_ftrace_function':

>> kernel/trace/ftrace.c:8197:14: warning: variable 'direct_mutex_locked' set but not used [-Wunused-but-set-variable]
    8197 |         bool direct_mutex_locked = false;
         |              ^~~~~~~~~~~~~~~~~~~

vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c

8182
8183 /**
8184 * register_ftrace_function - register a function for profiling
8185 * @ops: ops structure that holds the function for profiling.
8186 *
8187 * Register a function to be called by all functions in the
8188 * kernel.
8189 *
8190 * Note: @ops->func and all the functions it calls must be labeled
8191 * with "notrace", otherwise it will go into a
8192 * recursive loop.
8193 */
8194 int register_ftrace_function(struct ftrace_ops *ops)
8195 __releases(&direct_mutex)
8196 {

> 8197		bool direct_mutex_locked = false;

8198 int ret;
8199
8200 ftrace_ops_init(ops);
8201
8202 ret = prepare_direct_functions_for_ipmodify(ops);
8203 if (ret < 0)
8204 return ret;
8205 else if (ret == 1)
8206 direct_mutex_locked = true;
8207
8208 mutex_lock(&ftrace_lock);
8209
8210 ret = ftrace_startup(ops, 0);
8211
8212 mutex_unlock(&ftrace_lock);
8213

Signature · 2 lines

0-DAY CI Kernel Test Service
https://01.org/lkp

Petr Mladek

maintainer

2022-07-18 13:19 UTC #5 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

Quoted 48 lines · kernel test robot

Hi Song,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220718/202207181552.VuKfz9zg-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
        git checkout 9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
If you fix the issue, kindly add following tag where applicable
Reported-by kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
kernel/trace/ftrace.c: In function 'register_ftrace_function':
Quoted 1 line
Quoted 1 line
kernel/trace/ftrace.c:8197:14: warning: variable 'direct_mutex_locked' set but not used [-Wunused-but-set-variable]
    8197 |         bool direct_mutex_locked = false;
         |              ^~~~~~~~~~~~~~~~~~~
vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
8182
8183 /**
8184 * register_ftrace_function - register a function for profiling
8185 * @ops: ops structure that holds the function for profiling.
8186 *
8187 * Register a function to be called by all functions in the
8188 * kernel.
8189 *
8190 * Note: @ops->func and all the functions it calls must be labeled
8191 * with "notrace", otherwise it will go into a
8192 * recursive loop.
8193 */
8194 int register_ftrace_function(struct ftrace_ops *ops)
8195 __releases(&direct_mutex)
8196 {
Quoted 1 line
8197 bool direct_mutex_locked = false;

Quoted 9 lines

8198 int ret;
8199
8200 ftrace_ops_init(ops);
8201
8202 ret = prepare_direct_functions_for_ipmodify(ops);
8203 if (ret < 0)
8204 return ret;
8205 else if (ret == 1)
8206 direct_mutex_locked = true;

Honestly, this is another horrible trick. Would it be possible to call prepare_direct_functions_for_ipmodify() with direct_mutex already taken?

I mean something like:

	mutex_lock(&direct_mutex);

	ret = prepare_direct_functions_for_ipmodify(ops);
	if (ret)
		goto out:

	mutex_lock(&ftrace_lock);
	ret = ftrace_startup(ops, 0);
	mutex_unlock(&ftrace_lock);

out:

	mutex_unlock(&direct_mutex);
	return ret;

Quoted 6 lines

  8208		mutex_lock(&ftrace_lock);
  8209	
  8210		ret = ftrace_startup(ops, 0);
  8211	
  8212		mutex_unlock(&ftrace_lock);
  8213

Would be possible to handle tr->mutex the same way to avoid the trylock? I mean to take it in advance before direct_mutex?

Best Regards,
Petr

Song Liu

2022-07-18 16:59 UTC #6 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

Quoted 79 lines · Petr Mladek

Quoted 48 lines · kernel test robot
Hi Song,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220718/202207181552.VuKfz9zg-lkp(a)intel.com/config )
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
       # https://github.com/intel-lab-lkp/linux/commit/9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
       git remote add linux-review https://github.com/intel-lab-lkp/linux
       git fetch --no-tags linux-review Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
       git checkout 9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
       # save the config file
       mkdir build_dir && cp config build_dir/.config
       make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
If you fix the issue, kindly add following tag where applicable
Reported-by kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
kernel/trace/ftrace.c: In function 'register_ftrace_function':
Quoted 1 line
Quoted 1 line
kernel/trace/ftrace.c:8197:14: warning: variable 'direct_mutex_locked' set but not used [-Wunused-but-set-variable]
   8197 |         bool direct_mutex_locked = false;
        |              ^~~~~~~~~~~~~~~~~~~
vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
8182
8183 /**
8184 * register_ftrace_function - register a function for profiling
8185 * @ops: ops structure that holds the function for profiling.
8186 *
8187 * Register a function to be called by all functions in the
8188 * kernel.
8189 *
8190 * Note: @ops->func and all the functions it calls must be labeled
8191 * with "notrace", otherwise it will go into a
8192 * recursive loop.
8193 */
8194 int register_ftrace_function(struct ftrace_ops *ops)
8195 __releases(&direct_mutex)
8196 {
Quoted 1 line
8197 bool direct_mutex_locked = false;
Quoted 9 lines
8198 int ret;
8199
8200 ftrace_ops_init(ops);
8201
8202 ret = prepare_direct_functions_for_ipmodify(ops);
8203 if (ret < 0)
8204 return ret;
8205 else if (ret == 1)
8206 direct_mutex_locked = true;
Honestly, this is another horrible trick. Would it be possible to call prepare_direct_functions_for_ipmodify() with direct_mutex already taken?
I mean something like:
	mutex_lock(&direct_mutex);

	ret = prepare_direct_functions_for_ipmodify(ops);
	if (ret)
		goto out:

	mutex_lock(&ftrace_lock);
	ret = ftrace_startup(ops, 0);
	mutex_unlock(&ftrace_lock);

Quoted 3 lines

out:

	mutex_unlock(&direct_mutex);
	return ret;

Yeah, we can actually do something like this. We can also move the ops->flags & FTRACE_OPS_FL_IPMODIFY check to register_ftrace_function(), so we only lock direct_mutex when when it is necessary.

Quoted 11 lines

Quoted 8 lines
8208 mutex_lock(&ftrace_lock);
8209
8210 ret = ftrace_startup(ops, 0);
8211
8212 mutex_unlock(&ftrace_lock);
8213
Would be possible to handle tr->mutex the same way to avoid the trylock? I mean to take it in advance before direct_mutex?

Unfortunately, we cannot do this. ftrace code cannot look up bpf trampolines without locking direct_mutex.

Thanks,
Song

Steven Rostedt

maintainer

2022-07-19 18:32 UTC #7 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

Quoted 28 lines · On Mon, 18 Jul 2022 16:59:51 +0000 Song Liu <[email protected]> wrote:

Quoted 28 lines
Quoted 27 lines
vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
8182
8183 /**
8184 * register_ftrace_function - register a function for profiling
8185 * @ops: ops structure that holds the function for profiling.
8186 *
8187 * Register a function to be called by all functions in the
8188 * kernel.
8189 *
8190 * Note: @ops->func and all the functions it calls must be labeled
8191 * with "notrace", otherwise it will go into a
8192 * recursive loop.
8193 */
8194 int register_ftrace_function(struct ftrace_ops *ops)
8195 __releases(&direct_mutex)
8196 {
Quoted 1 line
8197 bool direct_mutex_locked = false;
8198 int ret;
8199
8200 ftrace_ops_init(ops);
8201
8202 ret = prepare_direct_functions_for_ipmodify(ops);
8203 if (ret < 0)
8204 return ret;
8205 else if (ret == 1)
8206 direct_mutex_locked = true;

Quoted 3 lines

Quoted 3 lines
Honestly, this is another horrible trick. Would it be possible to call prepare_direct_functions_for_ipmodify() with direct_mutex already taken?

Agreed. I'm not sure why I didn't notice this in the other versions. Probably was looking too much at the other logic. :-/

Quoted 13 lines

I mean something like:

	mutex_lock(&direct_mutex);

	ret = prepare_direct_functions_for_ipmodify(ops);
	if (ret)
		goto out:

	mutex_lock(&ftrace_lock);
	ret = ftrace_startup(ops, 0);
	mutex_unlock(&ftrace_lock);

Quoted 8 lines

Quoted 3 lines
out:
	mutex_unlock(&direct_mutex);
	return ret;  
Yeah, we can actually do something like this. We can also move the ops->flags & FTRACE_OPS_FL_IPMODIFY check to register_ftrace_function(), so we only lock direct_mutex when when it is necessary.

No need. Just take the direct_mutex, and perhaps add a:

	lockdep_assert_held(&direct_mutex);

in the prepare_direct_functions_for_ipmodify().

This is far from a fast path to do any tricks in trying to optimize it.

-- Steve

Song Liu

2022-07-19 22:28 UTC #8 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

Quoted 64 lines · Steven Rostedt

Quoted 28 lines · On Mon, 18 Jul 2022 16:59:51 +0000 Song Liu <[email protected]> wrote:
Quoted 28 lines
Quoted 27 lines
vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
8182
8183 /**
8184 * register_ftrace_function - register a function for profiling
8185 * @ops: ops structure that holds the function for profiling.
8186 *
8187 * Register a function to be called by all functions in the
8188 * kernel.
8189 *
8190 * Note: @ops->func and all the functions it calls must be labeled
8191 * with "notrace", otherwise it will go into a
8192 * recursive loop.
8193 */
8194 int register_ftrace_function(struct ftrace_ops *ops)
8195 __releases(&direct_mutex)
8196 {
Quoted 1 line
8197 bool direct_mutex_locked = false;
8198 int ret;
8199
8200 ftrace_ops_init(ops);
8201
8202 ret = prepare_direct_functions_for_ipmodify(ops);
8203 if (ret < 0)
8204 return ret;
8205 else if (ret == 1)
8206 direct_mutex_locked = true;
Quoted 3 lines
Quoted 3 lines
Honestly, this is another horrible trick. Would it be possible to call prepare_direct_functions_for_ipmodify() with direct_mutex already taken?
Agreed. I'm not sure why I didn't notice this in the other versions. Probably was looking too much at the other logic. :-/
Quoted 13 lines
Quoted 13 lines
I mean something like:
	mutex_lock(&direct_mutex);

	ret = prepare_direct_functions_for_ipmodify(ops);
	if (ret)
		goto out:

	mutex_lock(&ftrace_lock);
	ret = ftrace_startup(ops, 0);
	mutex_unlock(&ftrace_lock);
Quoted 8 lines
Quoted 3 lines
out:
	mutex_unlock(&direct_mutex);
	return ret;  
Yeah, we can actually do something like this. We can also move the ops->flags & FTRACE_OPS_FL_IPMODIFY check to register_ftrace_function(), so we only lock direct_mutex when when it is necessary.

Quoted 7 lines

No need. Just take the direct_mutex, and perhaps add a:
	lockdep_assert_held(&direct_mutex);
in the prepare_direct_functions_for_ipmodify().
This is far from a fast path to do any tricks in trying to optimize it.

Got it. I will fix these and send v5.

Thanks,
Song

Steven Rostedt

maintainer

2022-07-19 18:29 UTC #9 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

In reply to Song Liu

On Sun, 17 Jul 2022 22:54:47 -0700
Song Liu <[email protected]> wrote:

Again, make the subject:

ftrace: Allow IPMODIFY and DIRECT ops on the same function

Quoted 32 lines

include/linux/ftrace.h +24 -0

index acb35243ce5d..306bf08acda6 100644
@@ -208,6 +208,43 @@ enum {
 	FTRACE_OPS_FL_DIRECT			= BIT(17),
 };
 
+/*
+ * FTRACE_OPS_CMD_* commands allow the ftrace core logic to request changes
+ * to a ftrace_ops. Note, the requests may fail.
+ *
+ * ENABLE_SHARE_IPMODIFY_SELF - enable a DIRECT ops to work on the same
+ *                              function as an ops with IPMODIFY. Called
+ *                              when the DIRECT ops is being registered.
+ *                              This is called with both direct_mutex and
+ *                              ftrace_lock are locked.
+ *
+ * ENABLE_SHARE_IPMODIFY_PEER - enable a DIRECT ops to work on the same
+ *                              function as an ops with IPMODIFY. Called
+ *                              when the other ops (the one with IPMODIFY)
+ *                              is being registered.
+ *                              This is called with direct_mutex locked.
+ *
+ * DISABLE_SHARE_IPMODIFY_PEER - disable a DIRECT ops to work on the same
+ *                               function as an ops with IPMODIFY. Called
+ *                               when the other ops (the one with IPMODIFY)
+ *                               is being unregistered.
+ *                               This is called with direct_mutex locked.
+ */
+enum ftrace_ops_cmd {
+	FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_SELF,

include/linux/ftrace.h enum { · line 244

Quoted 10 lines

include/linux/ftrace.h +10 -0

+	FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_PEER,
+	FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY_PEER,
+};
+
+/*
+ * For most ftrace_ops_cmd,
+ * Returns:
+ *        0 - Success.
+ *        -EBUSY - The operation cannot process
+ *        -EAGAIN - The operation cannot process tempoorarily.

Just state:

Returns:
0 - Success
Negative on failure. The return value is dependent on the callback.

Let's not bind policy of the callback with ftrace.

Quoted 48 lines

include/linux/ftrace.h +4 -0

+ */
+typedef int (*ftrace_ops_func_t)(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 /* The hash used to know what functions callbacks trace */
 struct ftrace_ops_hash {

@@ -250,6 +287,7 @@ struct ftrace_ops {
 	unsigned long			trampoline;
 	unsigned long			trampoline_size;
 	struct list_head		list;
+	ftrace_ops_func_t		ops_func;
 #endif
 };

kernel/trace/ftrace.c +10 -0

index 0c15ec997c13..f52efbd13e51 100644
@@ -1861,6 +1861,8 @@ static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
 	ftrace_hash_rec_update_modify(ops, filter_hash, 1);
 }
 
+static bool ops_references_ip(struct ftrace_ops *ops, unsigned long ip);
+
 /*
  * Try to update IPMODIFY flag on each ftrace_rec. Return 0 if it is OK
  * or no-needed to update, -EBUSY if it detects a conflict of the flag

@@ -1869,6 +1871,13 @@ static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
  *  - If the hash is NULL, it hits all recs (if IPMODIFY is set, this is rejected)
  *  - If the hash is EMPTY_HASH, it hits nothing
  *  - Anything else hits the recs which match the hash entries.
+ *
+ * DIRECT ops does not have IPMODIFY flag, but we still need to check it
+ * against functions with FTRACE_FL_IPMODIFY. If there is any overlap, call
+ * ops_func(SHARE_IPMODIFY_SELF) to make sure current ops can share with
+ * IPMODIFY. If ops_func(SHARE_IPMODIFY_SELF) returns non-zero, propagate
+ * the return value to the caller and eventually to the owner of the DIRECT
+ * ops.
  */
 static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 					 struct ftrace_hash *old_hash,

@@ -1877,17 +1886,23 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec, *end = NULL;
 	int in_old, in_new;
+	bool is_ipmodify, is_direct;
 
 	/* Only update if the ops has been registered */

kernel/trace/ftrace.c static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops, · line 1900

Quoted 10 lines

kernel/trace/ftrace.c +5 -1

 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return 0;
 
-	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+	is_ipmodify = ops->flags & FTRACE_OPS_FL_IPMODIFY;
+	is_direct = ops->flags & FTRACE_OPS_FL_DIRECT;
+
+	/* either IPMODIFY nor DIRECT, skip */
+	if (!is_ipmodify && !is_direct)
 		return 0;

I wonder if we should also add:

	if (WARN_ON_ONCE(is_ipmodify && is_direct))
		return 0;

As a direct should never have an ipmodify.

Quoted 16 lines

kernel/trace/ftrace.c +3 -4

 	/*
-	 * Since the IPMODIFY is a very address sensitive action, we do not
-	 * allow ftrace_ops to set all functions to new hash.
+	 * Since the IPMODIFY and DIRECT are very address sensitive
+	 * actions, we do not allow ftrace_ops to set all functions to new
+	 * hash.
 	 */
 	if (!new_hash || !old_hash)
 		return -EINVAL;

@@ -1905,12 +1920,30 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 			continue;
 
 		if (in_new) {
-			/* New entries must ensure no others are using it */
-			if (rec->flags & FTRACE_FL_IPMODIFY)

kernel/trace/ftrace.c static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops, · line 1929

Quoted 10 lines

kernel/trace/ftrace.c +7 -3

-				goto rollback;
-			rec->flags |= FTRACE_FL_IPMODIFY;
-		} else /* Removed entry */
+			if (rec->flags & FTRACE_FL_IPMODIFY) {
+				int ret;
+
+				/* Cannot have two ipmodify on same rec */
+				if (is_ipmodify)
+					goto rollback;
+

I might add a

				FTRACE_WARN_ON(rec->flags &
				FTRACE_FL_DIRECT);

Just to be safe.

That is, if this is true, we are adding a new direct function to a record that already has one.

Quoted 103 lines

kernel/trace/ftrace.c +46 -8

+				/*
+				 * Another ops with IPMODIFY is already
+				 * attached. We are now attaching a direct
+				 * ops. Run SHARE_IPMODIFY_SELF, to check
+				 * whether sharing is supported.
+				 */
+				if (!ops->ops_func)
+					return -EBUSY;
+				ret = ops->ops_func(ops, FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_SELF);
+				if (ret)
+					return ret;
+			} else if (is_ipmodify) {
+				rec->flags |= FTRACE_FL_IPMODIFY;
+			}
+		} else if (is_ipmodify) {
 			rec->flags &= ~FTRACE_FL_IPMODIFY;
+		}
 	} while_for_each_ftrace_rec();
 
 	return 0;

@@ -2455,7 +2488,7 @@ static void call_direct_funcs(unsigned long ip, unsigned long pip,
 struct ftrace_ops direct_ops = {
 	.func		= call_direct_funcs,
 	.flags		= FTRACE_OPS_FL_IPMODIFY
-			  | FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS
+			  | FTRACE_OPS_FL_SAVE_REGS
 			  | FTRACE_OPS_FL_PERMANENT,
 	/*
 	 * By declaring the main trampoline as this trampoline

@@ -3072,14 +3105,14 @@ static inline int ops_traces_mod(struct ftrace_ops *ops)
 }
 
 /*
- * Check if the current ops references the record.
+ * Check if the current ops references the given ip.
  *
  * If the ops traces all functions, then it was already accounted for.
  * If the ops does not trace the current record function, skip it.
  * If the ops ignores the function via notrace filter, skip it.
  */
-static inline bool
-ops_references_rec(struct ftrace_ops *ops, struct dyn_ftrace *rec)
+static bool
+ops_references_ip(struct ftrace_ops *ops, unsigned long ip)
 {
 	/* If ops isn't enabled, ignore it */
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))

@@ -3091,16 +3124,29 @@ ops_references_rec(struct ftrace_ops *ops, struct dyn_ftrace *rec)
 
 	/* The function must be in the filter */
 	if (!ftrace_hash_empty(ops->func_hash->filter_hash) &&
-	    !__ftrace_lookup_ip(ops->func_hash->filter_hash, rec->ip))
+	    !__ftrace_lookup_ip(ops->func_hash->filter_hash, ip))
 		return false;
 
 	/* If in notrace hash, we ignore it too */
-	if (ftrace_lookup_ip(ops->func_hash->notrace_hash, rec->ip))
+	if (ftrace_lookup_ip(ops->func_hash->notrace_hash, ip))
 		return false;
 
 	return true;
 }
 
+/*
+ * Check if the current ops references the record.
+ *
+ * If the ops traces all functions, then it was already accounted for.
+ * If the ops does not trace the current record function, skip it.
+ * If the ops ignores the function via notrace filter, skip it.
+ */
+static bool
+ops_references_rec(struct ftrace_ops *ops, struct dyn_ftrace *rec)
+{
+	return ops_references_ip(ops, rec->ip);
+}
+
 static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
 {
 	bool init_nop = ftrace_need_init_nop();

@@ -5545,8 +5591,7 @@ int modify_ftrace_direct(unsigned long ip,
 }
 EXPORT_SYMBOL_GPL(modify_ftrace_direct);
 
-#define MULTI_FLAGS (FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_DIRECT | \
-		     FTRACE_OPS_FL_SAVE_REGS)
+#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS)
 
 static int check_direct_multi(struct ftrace_ops *ops)
 {

@@ -8004,6 +8049,137 @@ int ftrace_is_dead(void)
 	return ftrace_disabled;
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+/*
+ * When registering ftrace_ops with IPMODIFY, it is necessary to make sure
+ * it doesn't conflict with any direct ftrace_ops. If there is existing
+ * direct ftrace_ops on a kernel function being patched, call
+ * FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_PEER on it to enable sharing.
+ *
+ * @ops:     ftrace_ops being registered.
+ *
+ * Returns:

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8071

Quoted 10 lines

kernel/trace/ftrace.c +9 -0

+ *         0 - @ops does not have IPMODIFY or @ops itself is DIRECT, no
+ *             change needed;
+ *         1 - @ops has IPMODIFY, hold direct_mutex;
 
+ *         -EBUSY - currently registered DIRECT ftrace_ops cannot share the
+ *                  same function with IPMODIFY, abort the register.
+ *         -EAGAIN - cannot make changes to currently registered DIRECT
+ *                   ftrace_ops due to rare race conditions. Should retry
+ *                   later. This is needed to avoid potential deadlocks
+ *                   on the DIRECT ftrace_ops side.

Again, these are ops_func() specific and has nothing to do with the logic in this file. Just state:

* Returns:
* 0 - @ops does not have IPMODIFY or @ops itself is DIRECT, no
* change needed;
* 1 - @ops has IPMODIFY, hold direct_mutex;
* Negative on error.

And if we move the logic that this does not keep hold of the direct_mutex, we could just let the callback return any non-zero on error.

Quoted 18 lines

kernel/trace/ftrace.c +18 -0

+ */
+static int prepare_direct_functions_for_ipmodify(struct ftrace_ops *ops)
+	__acquires(&direct_mutex)
+{
+	struct ftrace_func_entry *entry;
+	struct ftrace_hash *hash;
+	struct ftrace_ops *op;
+	int size, i, ret;
+
+	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+		return 0;
+
+	mutex_lock(&direct_mutex);
+
+	hash = ops->func_hash->filter_hash;
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8098

Quoted 10 lines

kernel/trace/ftrace.c +10 -0

+			unsigned long ip = entry->ip;
+			bool found_op = false;
+
+			mutex_lock(&ftrace_lock);
+			do_for_each_ftrace_op(op, ftrace_ops_list) {
+				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
+					continue;
+				if (ops_references_ip(op, ip)) {
+					found_op = true;
+					break;

I think you want a goto here. The macros "do_for_each_ftrace_op() { .. } while_for_each_ftrace_op()" is a double loop. The break just moves to the next set of pages and does not break out of the outer loop.

					goto out_loop;

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8101

Quoted 2 lines

kernel/trace/ftrace.c +2 -0
+				}
+			} while_for_each_ftrace_op(op);

out_loop:

Quoted 47 lines

kernel/trace/ftrace.c +47 -0

+			mutex_unlock(&ftrace_lock);
+
+			if (found_op) {
+				if (!op->ops_func) {
+					ret = -EBUSY;
+					goto err_out;
+				}
+				ret = op->ops_func(op, FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_PEER);
+				if (ret)
+					goto err_out;
+			}
+		}
+	}
+
+	/*
+	 * Didn't find any overlap with direct ftrace_ops, or the direct
+	 * function can share with ipmodify. Hold direct_mutex to make sure
+	 * this doesn't change until we are done.
+	 */
+	return 1;
+
+err_out:
+	mutex_unlock(&direct_mutex);
+	return ret;
+}
+
+/*
+ * Similar to prepare_direct_functions_for_ipmodify, clean up after ops
+ * with IPMODIFY is unregistered. The cleanup is optional for most DIRECT
+ * ops.
+ */
+static void cleanup_direct_functions_after_ipmodify(struct ftrace_ops *ops)
+{
+	struct ftrace_func_entry *entry;
+	struct ftrace_hash *hash;
+	struct ftrace_ops *op;
+	int size, i;
+
+	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+		return;
+
+	mutex_lock(&direct_mutex);
+
+	hash = ops->func_hash->filter_hash;
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8157

Quoted 10 lines

kernel/trace/ftrace.c +10 -0

+			unsigned long ip = entry->ip;
+			bool found_op = false;
+
+			mutex_lock(&ftrace_lock);
+			do_for_each_ftrace_op(op, ftrace_ops_list) {
+				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
+					continue;
+				if (ops_references_ip(op, ip)) {
+					found_op = true;
+					break;

					goto out_loop;

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8160

Quoted 2 lines

kernel/trace/ftrace.c +2 -0
+				}
+			} while_for_each_ftrace_op(op);

out_loop:

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8162

Quoted 3 lines

kernel/trace/ftrace.c +3 -0

+			mutex_unlock(&ftrace_lock);
+
+			/* The cleanup is optional, iggore any errors */

"ignore"

Quoted 24 lines

kernel/trace/ftrace.c +20 -0

+			if (found_op && op->ops_func)
+				op->ops_func(op, FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY_PEER);
+		}
+	}
+	mutex_unlock(&direct_mutex);
+}
+
+#else  /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
+
+static int prepare_direct_functions_for_ipmodify(struct ftrace_ops *ops)
+{
+	return 0;
+}
+
+static void cleanup_direct_functions_after_ipmodify(struct ftrace_ops *ops)
+{
+}
+
+#endif  /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
+
 /**
  * register_ftrace_function - register a function for profiling

* @ops: ops structure that holds the function for profiling.

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8201

Quoted 10 lines

kernel/trace/ftrace.c +2 -0

@@ -8016,17 +8192,29 @@ int ftrace_is_dead(void)
  *       recursive loop.
  */
 int register_ftrace_function(struct ftrace_ops *ops)
+	__releases(&direct_mutex)
 {
+	bool direct_mutex_locked = false;
 	int ret;
 
 	ftrace_ops_init(ops);

I agree with Petr.

Just grab the direct_mutex_lock here.

	mutex_lock(&direct_mutex);

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8204

Quoted 3 lines

kernel/trace/ftrace.c +3 -0

+	ret = prepare_direct_functions_for_ipmodify(ops);
+	if (ret < 0)
+		return ret;

		goto out_unlock;

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8207

Quoted 3 lines

kernel/trace/ftrace.c +3 -0

+	else if (ret == 1)
+		direct_mutex_locked = true;
+

Nuke the above;

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8217

Quoted 10 lines

kernel/trace/ftrace.c +4 -0

 	mutex_lock(&ftrace_lock);
 
 	ret = ftrace_startup(ops, 0);
 
 	mutex_unlock(&ftrace_lock);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+	if (direct_mutex_locked)
+		mutex_unlock(&direct_mutex);
+#endif

Change this to:

out_unlock:

	mutex_unlock(&direct_mutex);

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8219

Quoted 2 lines

kernel/trace/ftrace.c +0 -0
 	return ret;
 }

-- Steve

Quoted 9 lines

kernel/trace/ftrace.c +1 -0

 EXPORT_SYMBOL_GPL(register_ftrace_function);

@@ -8045,6 +8233,7 @@ int unregister_ftrace_function(struct ftrace_ops *ops)
 	ret = ftrace_shutdown(ops, 0);
 	mutex_unlock(&ftrace_lock);
 
+	cleanup_direct_functions_after_ipmodify(ops);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(unregister_ftrace_function);

Steven Rostedt

maintainer

2022-07-19 18:39 UTC #10 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

Quoted 9 lines · On Tue, 19 Jul 2022 14:28:56 -0400 Steven Rostedt <[email protected]> wrote:

kernel/trace/ftrace.c static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops, · line 1929
Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+				/* Cannot have two ipmodify on same rec */
+				if (is_ipmodify)
+					goto rollback;
+  
I might add a
				FTRACE_WARN_ON(rec->flags &
				FTRACE_FL_DIRECT);

Bah, my email client line wrapped this. It was suppose to be:

FTRACE_WARN_ON(rec->flags & FTRACE_FL_DIRECT);

Just so you don't think I wanted that initial formatting ;-)

-- Steve

Quoted 5 lines

Just to be safe.
That is, if this is true, we are adding a new direct function to a record that already has one.

Song Liu

2022-07-19 22:58 UTC #11 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

In reply to Steven Rostedt

Quoted 11 lines · Steven Rostedt

On Sun, 17 Jul 2022 22:54:47 -0700
Song Liu <[email protected]> wrote:
Again, make the subject:
ftrace: Allow IPMODIFY and DIRECT ops on the same function

Will fix.

Quoted 12 lines

[...]
include/linux/ftrace.h enum { · line 244
Quoted 7 lines
include/linux/ftrace.h +7 -0
+
+/*
+ * For most ftrace_ops_cmd,
+ * Returns:
+ *        0 - Success.
+ *        -EBUSY - The operation cannot process
+ *        -EAGAIN - The operation cannot process tempoorarily.
Just state:

Quoted 6 lines

Returns:
0 - Success
Negative on failure. The return value is dependent on the callback.
Let's not bind policy of the callback with ftrace.

Will fix.

Quoted 7 lines

Quoted 7 lines
+ */
include/linux/ftrace.h +2 -0
+typedef int (*ftrace_ops_func_t)(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
+
#ifdef CONFIG_DYNAMIC_FTRACE
/* The hash used to know what functions callbacks trace */

[...]

Quoted 11 lines

Quoted 8 lines

kernel/trace/ftrace.c +5 -1

-	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+	is_ipmodify = ops->flags & FTRACE_OPS_FL_IPMODIFY;
+	is_direct = ops->flags & FTRACE_OPS_FL_DIRECT;
+
+	/* either IPMODIFY nor DIRECT, skip */
+	if (!is_ipmodify && !is_direct)

		return 0;

I wonder if we should also add:

Quoted 4 lines

	if (WARN_ON_ONCE(is_ipmodify && is_direct))
		return 0;
As a direct should never have an ipmodify.

Right, I will also remove IPMODIFY from direct_ops:

@ -2487,8 +2490,7 @@ static void call_direct_funcs(unsigned long ip, unsigned long pip,

struct ftrace_ops direct_ops = {
.func = call_direct_funcs,
- .flags = FTRACE_OPS_FL_IPMODIFY
- | FTRACE_OPS_FL_SAVE_REGS
+ .flags = FTRACE_OPS_FL_SAVE_REGS
| FTRACE_OPS_FL_PERMANENT,
/*
* By declaring the main trampoline as this trampoline

Quoted 8 lines

/*

kernel/trace/ftrace.c +3 -2

-	 * Since the IPMODIFY is a very address sensitive action, we do not
-	 * allow ftrace_ops to set all functions to new hash.
+	 * Since the IPMODIFY and DIRECT are very address sensitive
+	 * actions, we do not allow ftrace_ops to set all functions to new
+	 * hash.

[...]

Quoted 37 lines

Again, these are ops_func() specific and has nothing to do with the logic in this file. Just state:
* Returns:
* 0 - @ops does not have IPMODIFY or @ops itself is DIRECT, no
* change needed;
* 1 - @ops has IPMODIFY, hold direct_mutex;
* Negative on error.
And if we move the logic that this does not keep hold of the direct_mutex, we could just let the callback return any non-zero on error.
Quoted 24 lines
+ */
kernel/trace/ftrace.c +23 -0
+static int prepare_direct_functions_for_ipmodify(struct ftrace_ops *ops)
+	__acquires(&direct_mutex)
+{
+	struct ftrace_func_entry *entry;
+	struct ftrace_hash *hash;
+	struct ftrace_ops *op;
+	int size, i, ret;
+
+	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+		return 0;
+
+	mutex_lock(&direct_mutex);
+
+	hash = ops->func_hash->filter_hash;
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
+			unsigned long ip = entry->ip;
+			bool found_op = false;
+
+			mutex_lock(&ftrace_lock);
+			do_for_each_ftrace_op(op, ftrace_ops_list) {
+				if (!(op->flags & FTRACE_OPS_FL_DIRECT))

Quoted 8 lines

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8098
Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+					continue;
+				if (ops_references_ip(op, ip)) {
+					found_op = true;
+					break;
I think you want a goto here. The macros "do_for_each_ftrace_op() { .. } while_for_each_ftrace_op()" is a double loop. The break just moves to the next set of pages and does not break out of the outer loop.

Hmmm... really? I didn't see it ...

#define do_for_each_ftrace_op(op, list)                 \
        op = rcu_dereference_raw_check(list);                   \
        do

#define while_for_each_ftrace_op(op)                            \
        while (likely(op = rcu_dereference_raw_check((op)->next)) &&    \
               unlikely((op) != &ftrace_list_end))

Did I miss something...?

Quoted 5 lines

					goto out_loop;

Quoted 2 lines

(quoted diff) +2 -0

+				}
+			} while_for_each_ftrace_op(op);

[...]

Quoted 14 lines

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8217
Quoted 11 lines
	mutex_lock(&ftrace_lock);

	ret = ftrace_startup(ops, 0);

	mutex_unlock(&ftrace_lock);
kernel/trace/ftrace.c +4 -0
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+	if (direct_mutex_locked)
+		mutex_unlock(&direct_mutex);
+#endif
Change this to:

Quoted 3 lines

out_unlock:

	mutex_unlock(&direct_mutex);

We still need #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, as direct_mutex is not defined without that config.

Thanks,
Song

[...]

Steven Rostedt

maintainer

2022-07-19 23:04 UTC #12 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

Quoted 26 lines · On Tue, 19 Jul 2022 22:57:52 +0000 Song Liu <[email protected]> wrote:

Quoted 10 lines
Quoted 10 lines
kernel/trace/ftrace.c +10 -0
+	hash = ops->func_hash->filter_hash;
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
+			unsigned long ip = entry->ip;
+			bool found_op = false;
+
+			mutex_lock(&ftrace_lock);
+			do_for_each_ftrace_op(op, ftrace_ops_list) {
+				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
Quoted 8 lines
kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8098
Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+					continue;
+				if (ops_references_ip(op, ip)) {
+					found_op = true;
+					break;  
I think you want a goto here. The macros "do_for_each_ftrace_op() { .. } while_for_each_ftrace_op()" is a double loop. The break just moves to the next set of pages and does not break out of the outer loop.
Hmmm... really? I didn't see it ...
#define do_for_each_ftrace_op(op, list)                 \
        op = rcu_dereference_raw_check(list);                   \
        do

Quoted 5 lines

#define while_for_each_ftrace_op(op)                            \
        while (likely(op = rcu_dereference_raw_check((op)->next)) &&    \
               unlikely((op) != &ftrace_list_end))

Did I miss something...?

Bah, you're right. I was confusing it with do_for_each_ftrace_rec(), which *is* a double loop.

Never mind ;-)

Quoted 23 lines

Quoted 6 lines
					goto out_loop;
Quoted 2 lines
(quoted diff) +2 -0
+				}
+			} while_for_each_ftrace_op(op);  
[...]
Quoted 14 lines
kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8217
Quoted 11 lines
  
	mutex_lock(&ftrace_lock);

	ret = ftrace_startup(ops, 0);

	mutex_unlock(&ftrace_lock);
kernel/trace/ftrace.c +4 -0
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+	if (direct_mutex_locked)
+		mutex_unlock(&direct_mutex);
+#endif  
Change this to:

Quoted 6 lines

Quoted 3 lines
out_unlock:
	mutex_unlock(&direct_mutex);
We still need #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, as direct_mutex is not defined without that config.

Ah, right. I just meant to get rid of the if statement.

To keep the code clean, perhaps we should have:

#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS

# define lock_direct_mutex()	mutex_lock(&direct_mutex)
# define unlock_direct_mutex()	mutex_unlock(&direct_mutex)
#else
# define lock_direct_mutex()	do { } while (0)
# define unlock_direct_mutex()	do { } while (0)
#endif

And use that here.

-- Steve

Song Liu

2022-07-19 23:24 UTC #13 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

In reply to Steven Rostedt

Quoted 16 lines · Steven Rostedt

Quoted 13 lines
/**
* register_ftrace_function - register a function for profiling
* @ops: ops structure that holds the function for profiling.
kernel/trace/ftrace.c +2 -0
@@ -8016,17 +8192,29 @@ int ftrace_is_dead(void)
 * recursive loop.
 */
 int register_ftrace_function(struct ftrace_ops *ops)
+	__releases(&direct_mutex)
 {
+	bool direct_mutex_locked = false;
 	int ret;
 

Quoted 8 lines

kernel/trace/ftrace.c int ftrace_is_dead(void) · line 8201
Quoted 2 lines
kernel/trace/ftrace.c +0 -0
 	ftrace_ops_init(ops);
 
I agree with Petr.
Just grab the direct_mutex_lock here.
	mutex_lock(&direct_mutex);

Actually, we cannot blindly lock direct_mutex here, as register_ftrace_direct() already locks it before calling register_ftrace_function(). We still need the if (IPMODIFY) check.

Thanks,
Song

Quoted 5 lines

Quoted 4 lines

kernel/trace/ftrace.c +3 -0

+	ret = prepare_direct_functions_for_ipmodify(ops);
+	if (ret < 0)
+		return ret;

Steven Rostedt

maintainer

2022-07-19 23:27 UTC #14 lore raw

Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

Quoted 4 lines · On Tue, 19 Jul 2022 23:24:35 +0000 Song Liu <[email protected]> wrote:

Actually, we cannot blindly lock direct_mutex here, as register_ftrace_direct() already locks it before calling register_ftrace_function(). We still need the if (IPMODIFY) check.

Let's not play games with this then.

Create a register_ftrace_function_nolock()

and use that for register_ftrace_direct().

Otherwise it's going to be a nightmare to keep track of.

-- Steve

Song Liu

author maintainer

2022-07-18 05:55 UTC #15 lore raw

PATCH v4 3/4 bpf, x64: Allow to use caller address from stack

In reply to Song Liu

From: Jiri Olsa <[email protected]>

Currently we call the original function by using the absolute address given at the JIT generation. That's not usable when having trampoline attached to multiple functions, or the target address changes dynamically (in case of live patch). In such cases we need to take the return address from the stack.

Adding support to retrieve the original function address from the stack by adding new BPF_TRAMP_F_ORIG_STACK flag for arch_prepare_bpf_trampoline function.

Basically we take the return address of the 'fentry' call:

function + 0: call fentry # stores 'function + 5' address on stack
function + 5: ...

The 'function + 5' address will be used as the address for the original function to call.

Signed-off-by Jiri Olsa <[email protected]>

Signed-off-by Song Liu <[email protected]>

---
 arch/x86/net/bpf_jit_comp.c | 13 +++++++++----
 include/linux/bpf.h         |  5 +++++
 2 files changed, 14 insertions(+), 4 deletions(-)

arch/x86/net/bpf_jit_comp.c +9 -4

index 54c7f46c453f..e1b0c5ed0b7c 100644
@@ -2119,10 +2119,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
 		restore_regs(m, &prog, nr_args, regs_off);
 
-		/* call original function */
-		if (emit_call(&prog, orig_call, prog)) {
-			ret = -EINVAL;
-			goto cleanup;
+		if (flags & BPF_TRAMP_F_ORIG_STACK) {
+			emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
+			EMIT2(0xff, 0xd0); /* call *rax */
+		} else {
+			/* call original function */
+			if (emit_call(&prog, orig_call, prog)) {
+				ret = -EINVAL;
+				goto cleanup;
+			}
 		}
 		/* remember return value in a stack for bpf prog to access */
 		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);

include/linux/bpf.h +5 -0

index a5bf00649995..7496842a4671 100644
@@ -751,6 +751,11 @@ struct btf_func_model {
 /* Return the return value of fentry prog. Only used by bpf_struct_ops. */
 #define BPF_TRAMP_F_RET_FENTRY_RET	BIT(4)
 
+/* Get original function from stack instead of from provided direct address.
+ * Makes sense for trampolines with fexit or fmod_ret programs.
+ */
+#define BPF_TRAMP_F_ORIG_STACK		BIT(5)
+
 /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
  * bytes on x86.
  */

Signature · 1 line

2.30.2

Song Liu

author maintainer

2022-07-18 05:55 UTC #16 lore raw

PATCH v4 4/4 bpf: support bpf_trampoline on functions with IPMODIFY (e.g. livepatch)

In reply to Song Liu

When tracing a function with IPMODIFY ftrace_ops (livepatch), the bpf trampoline must follow the instruction pointer saved on stack. This needs extra handling for bpf trampolines with BPF_TRAMP_F_CALL_ORIG flag.

Implement bpf_tramp_ftrace_ops_func and use it for the ftrace_ops used by BPF trampoline. This enables tracing functions with livepatch.

This also requires moving bpf trampoline to *_ftrace_direct_mult APIs.

Link https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Song Liu <[email protected]>

---
 include/linux/bpf.h     |   8 ++
 kernel/bpf/trampoline.c | 158 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 149 insertions(+), 17 deletions(-)

include/linux/bpf.h +8 -0

index 7496842a4671..f35c59e0b742 100644
@@ -47,6 +47,7 @@ struct kobject;
 struct mem_cgroup;
 struct module;
 struct bpf_func_state;
+struct ftrace_ops;
 
 extern struct idr btf_idr;
 extern spinlock_t btf_idr_lock;

@@ -756,6 +757,11 @@ struct btf_func_model {
  */
 #define BPF_TRAMP_F_ORIG_STACK		BIT(5)
 
+/* This trampoline is on a function with another ftrace_ops with IPMODIFY,
+ * e.g., a live patch. This flag is set and cleared by ftrace call backs,
+ */
+#define BPF_TRAMP_F_SHARE_IPMODIFY	BIT(6)
+
 /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
  * bytes on x86.
  */

@@ -838,9 +844,11 @@ struct bpf_tramp_image {
 struct bpf_trampoline {
 	/* hlist for trampoline_table */
 	struct hlist_node hlist;
+	struct ftrace_ops *fops;
 	/* serializes access to fields of this trampoline */
 	struct mutex mutex;
 	refcount_t refcnt;
+	u32 flags;
 	u64 key;
 	struct {
 		struct btf_func_model model;

kernel/bpf/trampoline.c +141 ~~-17~~

257 lines not on this page · raw message

Signature · 1 line

2.30.2