ftrace: host klp and bpf trampoline together

This enables users of ftrace_direct_multi to specify the flags based on the actual use case. For example, some users may not set flag IPMODIFY.

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

---
 kernel/trace/ftrace.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

kernel/trace/ftrace.c +2 -3

index 2fcd17857ff6..afe782ae28d3 100644
@@ -5456,8 +5456,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)
 {

@@ -5547,7 +5546,7 @@ int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 	}
 
 	ops->func = call_direct_funcs;
-	ops->flags = MULTI_FLAGS;
+	ops->flags |= MULTI_FLAGS;
 	ops->trampoline = FTRACE_REGS_ADDR;
 
 	err = register_ftrace_function(ops);

On Thu, 2 Jun 2022 12:37:02 -0700
Song Liu <[email protected]> wrote:

Quoted 2 lines

This enables users of ftrace_direct_multi to specify the flags based on the actual use case. For example, some users may not set flag IPMODIFY.

If we apply this patch without any of the others, then we are relying on the caller to get it right?

That is, can we register a direct function with this function and pick a function with IPMODIFY already attached?

-- Steve

Quoted 29 lines

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

---
kernel/trace/ftrace.c | 5 ++---

1 file changed, 2 insertions(+), 3 deletions(-)

kernel/trace/ftrace.c +2 -3

index 2fcd17857ff6..afe782ae28d3 100644
@@ -5456,8 +5456,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)
 {

@@ -5547,7 +5546,7 @@ int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
   }
 
   ops->func = call_direct_funcs;
-    ops->flags = MULTI_FLAGS;
+    ops->flags |= MULTI_FLAGS;
   ops->trampoline = FTRACE_REGS_ADDR;
 
   err = register_ftrace_function(ops);

Quoted 2 lines

That is, can we register a direct function with this function and pick a function with IPMODIFY already attached?

Yes, if the direct function follows regs->ip, it works.

For example, BPF trampoline with only fentry calls will just work with only this patch.

Quoted 6 lines · On Thu, 14 Jul 2022 00:11:53 +0000 Song Liu <[email protected]> wrote:

Quoted 2 lines

That is, can we register a direct function with this function and pick a function with IPMODIFY already attached?

Yes, if the direct function follows regs->ip, it works.

For example, BPF trampoline with only fentry calls will just work with only this patch.

I replied with my thoughts on this to patch 3, but I disagree with this.

ftrace has no idea if the direct trampoline modifies the IP or not. ftrace must assume that it does, and the management should be done in the infrastructure.

As I replied to patch 3, here's my thoughts.

DIRECT is treated as though it changes the IP. If you register it to a function that has an IPMODIFY already set to it, it will call the ops->ops_func() asking if the ops can use SHARED_IPMODIFY (which means a DIRECT can be shared with IPMODIFY). If it can, then it returns true, and the SHARED_IPMODIFY is set *by ftrace*. The user of the ftrace APIs should not have to manage this. It should be managed by the ftrace infrastructure.

Quoted 9 lines

As I replied to patch 3, here's my thoughts.

DIRECT is treated as though it changes the IP. If you register it to a function that has an IPMODIFY already set to it, it will call the ops->ops_func() asking if the ops can use SHARED_IPMODIFY (which means a DIRECT can be shared with IPMODIFY). If it can, then it returns true, and the SHARED_IPMODIFY is set *by ftrace*. The user of the ftrace APIs should not have to manage this. It should be managed by the ftrace infrastructure.

Hmm... I don't think this gonna work.

First, two IPMODIFY ftrace ops cannot work together on the same kernel function. So there won't be a ops with both IPMODIFY and SHARE_IPMODIFY.

non-direct ops without IPMODIFY can already share with IPMODIFY ops.

Only direct ops need SHARE_IPMODIFY flag, and it means "I can share with another ops with IPMODIFY". So there will be different flavors of direct ops:

1. w/ IPMODIFY, w/o SHARE_IPMODIFY;
2. w/o IPMODIFY, w/o SHARE_IPMODIFY;
3. w/o IPMODIFY, w/ SHARE_IPMODIFY.

#1 can never work on the same function with another IPMODIFY ops, and we don't plan to make it work. #2 does not work with another IPMODIFY ops. And #3 works with another IPMODIFY ops.

The owner of the direct trampoline uses these flags to tell ftrace infrastructure the property of this trampoline.

BPF trampolines with only fentry calls are #3 direct ops. BPF trampolines with fexit or fmod_ret calls will be #2 trampoline by default, but it is also possible to generate #3 trampoline for it.

BPF side will try to register #2 trampoline, If ftrace detects another IPMODIFY ops on the same function, it will let BPF trampoline know with -EAGAIN from register_ftrace_direct_multi(). Then, BPF side will regenerate a #3 trampoline and register it again.

I know this somehow changes the policy with direct ops, but it is the only way this can work, AFAICT.

Does this make sense? Did I miss something?

Quoted 10 lines · On Thu, 14 Jul 2022 01:42:59 +0000 Song Liu <[email protected]> wrote:

Quoted 9 lines

As I replied to patch 3, here's my thoughts.

DIRECT is treated as though it changes the IP. If you register it to a function that has an IPMODIFY already set to it, it will call the ops->ops_func() asking if the ops can use SHARED_IPMODIFY (which means a DIRECT can be shared with IPMODIFY). If it can, then it returns true, and the SHARED_IPMODIFY is set *by ftrace*. The user of the ftrace APIs should not have to manage this. It should be managed by the ftrace infrastructure.

Quoted 4 lines

Hmm... I don't think this gonna work.

First, two IPMODIFY ftrace ops cannot work together on the same kernel function. So there won't be a ops with both IPMODIFY and SHARE_IPMODIFY.

I'm not saying that.

I'm saying that ftrace does not have any clue (nor cares) about what a DIRECT ops does. It might modify the IP or it might not. It doesn't know.

But ftrace has control over the callbacks it does control.

A DIRECT ops knows if it can work with another ops that has IPMODIFY set. If the DIRECT ops does not work with IPMODIFY (perhaps it wants to modify the IP), then it will tell ftrace that it can't and ftrace will not allow it.

That is, ftrace doesn't care if the DIRECT ops modifies the IP or not. It can only ask (through the ops->ops_func()) if the direct trampoline can honor the IP that is modified. If it can, then it reports back that it can, and then ftrace will set that ops to SHARED_MODIFY, and the direct function can switch the ops->func() to one that uses SHARED_MODIFY.

Quoted 1 line

Quoted 1 line

non-direct ops without IPMODIFY can already share with IPMODIFY ops.

It can? ftrace sets IPMODIFY for all DIRECT callers to prevent that. Except for this patch that removes that restriction (which I believe is broken).

Quoted 4 lines

Only direct ops need SHARE_IPMODIFY flag, and it means "I can share with another ops with IPMODIFY". So there will be different flavors of direct ops:

I agree that only DIRECT ops can have SHARED_IPMODIFY set. That's what I'm saying. But I'm saying it gets set by ftrace.

Quoted 8 lines

1. w/ IPMODIFY, w/o SHARE_IPMODIFY;
2. w/o IPMODIFY, w/o SHARE_IPMODIFY;
3. w/o IPMODIFY, w/ SHARE_IPMODIFY.

#1 can never work on the same function with another IPMODIFY ops, and we don't plan to make it work. #2 does not work with another IPMODIFY ops. And #3 works with another IPMODIFY ops.

Lets look at this differently. What I'm proposing is that registering a direct ops does not need to tell ftrace if it modifies the IP or not. That's because ftrace doesn't care. Once ftrace calls the direct trampoline it loses all control. With the ftrace ops callbacks, it is the one responsible for setting up the modified IP. That's not the case with the direct trampolines.

I'm saying that ftrace doesn't care what the DIRECT ops does. But it will not, by default, allow an IPMODIFY to happen when a DIRECT ops is on the same function, or vice versa.

What I'm suggesting is when a IPMODIFY tries to attach to a function that also has a DIRECT ops, or a DIRECT tries to attach to a function that already has an IPMODIFY ops on it, that ftrace calls the direct ops->ops_func() asking if it is safe to use with an IPMODIFY function.

If the direct ops modifies the IP itself, it will return a "no", and ftrace will reject the attachment. If the direct ops can, it returns a "yes" and then ftrace will set the SHARED_IPMODIFY flag to that ops and continue.

The fact that the ops->ops_func was called will let the caller (bpf) know that it needs to use the stack to return to the function, or to call it if it is also tracing the return.

If the IPMODIFY ops is removed, then ftrace will call the ops->ops_func() telling it it no longer has the IPMODIFY set, and will clear the SHARED_IPMODIFY flag, and then the owner of the direct ops can go back to doing whatever it did before (the calling the function directly, or whatever).

Quoted 3 lines

The owner of the direct trampoline uses these flags to tell ftrace infrastructure the property of this trampoline.

I disagree. The owner shouldn't have to care about the flags. Let ftrace handle it. This will make things so much more simple for both BPF and ftrace.

Quoted 4 lines

BPF trampolines with only fentry calls are #3 direct ops. BPF trampolines with fexit or fmod_ret calls will be #2 trampoline by default, but it is also possible to generate #3 trampoline for it.

And ftrace doesn't care about this. But bpf needs to care if the IP is being modified or not. Which can be done by the ops->ops_func() that you added.

Quoted 5 lines

BPF side will try to register #2 trampoline, If ftrace detects another IPMODIFY ops on the same function, it will let BPF trampoline know with -EAGAIN from register_ftrace_direct_multi(). Then, BPF side will regenerate a #3 trampoline and register it again.

This is too complex. You are missing the simple way.

Quoted 3 lines

I know this somehow changes the policy with direct ops, but it is the only way this can work, AFAICT.

I disagree. There's a much better way that this can work.

Quoted 2 lines

Does this make sense? Did I miss something?

Let me start from the beginning.

1. Live kernel patching patches function foo.

2. lkp has an ops->flags | IPMODIFY set when it registers.

3. bpf registers a direct trampoline to function foo.

4. bpf has an ops->flags | DIRECT set when it registers

5. ftrace sees that the function already has an ops->flag = IPMODIFY on it, so it calls bpf ops->ops_func(SHARE_WITH_IPMODIFY)

6. bpf can and does the following

a. if it's the simple #1 trampoline (just traces the start of a function)
it doesn't need to do anything special returns "yes".

b. if it's the #2 trampoline, it will change the trampoline to use the
stack to find what to call, and returns "yes".

7. ftrace gets "yes" and sets the *ipmodify* ops with SHARED_IPMODIFY (there's a reason for setting this flag for the ipmodify ops and not the direct ops).

8. LKP is removed from the function foo.

9. ftrace sees the lkp IPMODIFY ops has SHARED_IPMODIFY on it, and knows that there's a direct call here too. It removes the IPMODIFY ops, and then calls the direct ops->ops_func(STOP_SHARE_WITH_IPMODIFY) to let the direct code know that it is no longer sharing with an IPMODIFY such that it can change to call the function directly and not use the stack.

Quoted 6 lines

Quoted 3 lines

non-direct ops without IPMODIFY can already share with IPMODIFY ops.

It can? ftrace sets IPMODIFY for all DIRECT callers to prevent that. Except for this patch that removes that restriction (which I believe is broken).

I mean "non-direct" ftrace ops, not direct ftrace ops.

Quoted 1 line

Let me start from the beginning.

I got your point now. We replace the flag on direct trampoline with a callback check. So yes, this works.

Quoted 5 lines

9. ftrace sees the lkp IPMODIFY ops has SHARED_IPMODIFY on it, and knows that there's a direct call here too. It removes the IPMODIFY ops, and then calls the direct ops->ops_func(STOP_SHARE_WITH_IPMODIFY) to let the direct code know that it is no longer sharing with an IPMODIFY such that it can change to call the function directly and not use the stack.

I wonder whether we still need this flag. Alternatively, we can always find direct calls on the function and calls ops_func(STOP_SHARE_WITH_IPMODIFY).

What do you think about this?

live patch and BPF trampoline (kfunc/kretfunc in bpftrace) are important features for modern systems. Currently, it is not possible to use live patch and BPF trampoline on the same kernel function at the same time. This is because of the resitriction that only one ftrace_ops with flag FTRACE_OPS_FL_IPMODIFY on the same kernel function.

BPF trampoline uses direct ftrace_ops, which assumes IPMODIFY. However, not all direct ftrace_ops would overwrite the actual function. This means it is possible to have a non-IPMODIFY direct ftrace_ops to share the same kernel function with an IPMODIFY ftrace_ops.

Introduce FTRACE_OPS_FL_SHARE_IPMODIFY, which allows the direct ftrace_ops to share with IPMODIFY ftrace_ops. With FTRACE_OPS_FL_SHARE_IPMODIFY flag set, the direct ftrace_ops would call the target function picked by the IPMODIFY ftrace_ops.

Comment "IPMODIFY, DIRECT, and SHARE_IPMODIFY" in include/linux/ftrace.h contains more information about how SHARE_IPMODIFY interacts with IPMODIFY and DIRECT flags.

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

---
 include/linux/ftrace.h |  74 +++++++++++++++++
 kernel/trace/ftrace.c  | 179 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 242 insertions(+), 11 deletions(-)

include/linux/ftrace.h +74 -0

index 9023bf69f675..bfacf608de9c 100644
@@ -98,6 +98,18 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val
 }
 #endif
 
+/*
+ * FTRACE_OPS_CMD_* commands allow the ftrace core logic to request changes
+ * to a ftrace_ops.
+ *
+ * ENABLE_SHARE_IPMODIFY - enable FTRACE_OPS_FL_SHARE_IPMODIFY.
+ * DISABLE_SHARE_IPMODIFY - disable FTRACE_OPS_FL_SHARE_IPMODIFY.
+ */
+enum ftrace_ops_cmd {
+	FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY,
+	FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY,
+};
+
 #ifdef CONFIG_FUNCTION_TRACER
 
 extern int ftrace_enabled;

@@ -189,6 +201,9 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
  *             ftrace_enabled.
  * DIRECT - Used by the direct ftrace_ops helper for direct functions
  *            (internal ftrace only, should not be used by others)
+ * SHARE_IPMODIFY - For direct ftrace_ops only. Set when the direct function
+ *            is ready to share same kernel function with IPMODIFY function
+ *            (live patch, etc.).
  */
 enum {
 	FTRACE_OPS_FL_ENABLED			= BIT(0),

@@ -209,8 +224,66 @@ enum {
 	FTRACE_OPS_FL_TRACE_ARRAY		= BIT(15),
 	FTRACE_OPS_FL_PERMANENT                 = BIT(16),
 	FTRACE_OPS_FL_DIRECT			= BIT(17),
+	FTRACE_OPS_FL_SHARE_IPMODIFY		= BIT(18),
 };
 
+/*
+ * IPMODIFY, DIRECT, and SHARE_IPMODIFY.
+ *
+ * ftrace provides IPMODIFY flag for users to replace existing kernel
+ * function with a different version. This is achieved by setting regs->ip.
+ * The top user of IPMODIFY is live patch.
+ *
+ * DIRECT allows user to load custom trampoline on top of ftrace. DIRECT
+ * ftrace does not overwrite regs->ip. Instead, the custom trampoline is
+ * saved separately (for example, orig_ax on x86). The top user of DIRECT
+ * is bpf trampoline.
+ *
+ * It is not super rare to have both live patch and bpf trampoline on the
+ * same kernel function. Therefore, it is necessary to allow the two work
+ * with each other. Given that IPMODIFY and DIRECT target addressese are
+ * saved separately, this is feasible, but we need to be careful.
+ *
+ * The policy between IPMODIFY and DIRECT is:
+ *
+ *  1. Each kernel function can only have one IPMODIFY ftrace_ops;
+ *  2. Each kernel function can only have one DIRECT ftrace_ops;
+ *  3. DIRECT ftrace_ops may have IPMODIFY or not;
+ *  4. Each kernel function may have one non-DIRECT IPMODIFY ftrace_ops,
+ *     and one non-IPMODIFY DIRECT ftrace_ops at the same time. This
+ *     requires support from the DIRECT ftrace_ops. Specifically, the
+ *     DIRECT trampoline should call the kernel function at regs->ip.
+ *     If the DIRECT ftrace_ops supports sharing a function with ftrace_ops
+ *     with IPMODIFY, it should set flag SHARE_IPMODIFY.
+ *
+ * Some DIRECT ftrace_ops has an option to enable SHARE_IPMODIFY or not.
+ * Usually, the non-SHARE_IPMODIFY option gives better performance. To take
+ * advantage of this performance benefit, is necessary to only enable
+ * SHARE_IPMODIFY only when it is on the same function as an IPMODIFY
+ * ftrace_ops. There are two cases to consider:
+ *
+ *  1. IPMODIFY ftrace_ops is registered first. When the (non-IPMODIFY, and
+ *     non-SHARE_IPMODIFY) DIRECT ftrace_ops is registered later,
+ *     register_ftrace_direct_multi() returns -EAGAIN. If the user of
+ *     the DIRECT ftrace_ops can support SHARE_IPMODIFY, it should enable
+ *     SHARE_IPMODIFY and retry.
+ *  2. (non-IPMODIFY, and non-SHARE_IPMODIFY) DIRECT ftrace_ops is
+ *     registered first. When the IPMODIFY ftrace_ops is registered later,
+ *     it is necessary to ask the direct ftrace_ops to enable
+ *     SHARE_IPMODIFY support. This is achieved via ftrace_ops->ops_func
+ *     cmd=FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY. For more details on this
+ *     condition, check out prepare_direct_functions_for_ipmodify().
+ */
+
+/*
+ * 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 {

@@ -253,6 +326,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 +112 -11

index 6a419f6bbbf0..868bbc753803 100644
@@ -1865,7 +1865,8 @@ static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
 /*
  * 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
- * on a ftrace_rec, and -EINVAL if the new_hash tries to trace all recs.
+ * on a ftrace_rec, -EINVAL if the new_hash tries to trace all recs, and
+ * -EAGAIN if the ftrace_ops need to enable SHARE_IPMODIFY.
  * Note that old_hash and new_hash has below meanings
  *  - 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

@@ -1875,6 +1876,7 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 					 struct ftrace_hash *old_hash,
 					 struct ftrace_hash *new_hash)
 {
+	bool is_ipmodify, is_direct, share_ipmodify;
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec, *end = NULL;
 	int in_old, in_new;

@@ -1883,7 +1885,24 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return 0;
 
-	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+	/*
+	 * The following are all the valid combinations of is_ipmodify,
+	 * is_direct, and share_ipmodify
+	 *
+	 *             is_ipmodify     is_direct     share_ipmodify
+	 *  #1              0               0                0
+	 *  #2              1               0                0
+	 *  #3              1               1                0
+	 *  #4              0               1                0
+	 *  #5              0               1                1
+	 */
+
+
+	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)   /* combinations #1 */
 		return 0;
 
 	/*

@@ -1893,6 +1912,30 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!new_hash || !old_hash)
 		return -EINVAL;
 
+	share_ipmodify = ops->flags & FTRACE_OPS_FL_SHARE_IPMODIFY;
+
+	/*
+	 * This ops itself doesn't do ip_modify and it can share a fentry
+	 * with other ops with ipmodify, nothing to do.
+	 */
+	if (!is_ipmodify && share_ipmodify)   /* combinations #5 */
+		return 0;
+
+	/*
+	 * Only three combinations of is_ipmodify, is_direct, and
+	 * share_ipmodify for the logic below:
+	 * #2 live patch
+	 * #3 direct with ipmodify
+	 * #4 direct without ipmodify
+	 *
+	 *             is_ipmodify     is_direct     share_ipmodify
+	 *  #2              1               0                0
+	 *  #3              1               1                0
+	 *  #4              0               1                0
+	 *
+	 * Only update/rollback rec->flags for is_ipmodify == 1 (#2 and #3)
+	 */
+
 	/* Update rec->flags */
 	do_for_each_ftrace_rec(pg, rec) {
 

@@ -1906,12 +1949,18 @@ 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)
-				goto rollback;
-			rec->flags |= FTRACE_FL_IPMODIFY;
-		} else /* Removed entry */
+			if (rec->flags & FTRACE_FL_IPMODIFY) {
+				/* cannot have two ipmodify on same rec */
+				if (is_ipmodify)  /* combination #2 and #3 */
+					goto rollback;
+				/* let user enable share_ipmodify and retry */
+				return  -EAGAIN;  /* combination #4 */
+			} else if (is_ipmodify) {
+				rec->flags |= FTRACE_FL_IPMODIFY;
+			}
+		} else if (is_ipmodify) {/* Removed entry */
 			rec->flags &= ~FTRACE_FL_IPMODIFY;
+		}
 	} while_for_each_ftrace_rec();
 
 	return 0;

@@ -3115,14 +3164,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)
+ops_references_ip(struct ftrace_ops *ops, unsigned long ip)
 {
 	/* If ops isn't enabled, ignore it */
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))

@@ -3134,16 +3183,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 inline 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();

@@ -5519,6 +5581,14 @@ int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 	if (ops->flags & FTRACE_OPS_FL_ENABLED)
 		return -EINVAL;
 
+	/*
+	 * if the ops does ipmodify, it cannot share the same fentry with
+	 * other functions with ipmodify.
+	 */
+	if ((ops->flags & FTRACE_OPS_FL_IPMODIFY) &&
+	    (ops->flags & FTRACE_OPS_FL_SHARE_IPMODIFY))
+		return -EINVAL;
+
 	hash = ops->func_hash->filter_hash;
 	if (ftrace_hash_empty(hash))
 		return -EINVAL;

@@ -7901,6 +7971,83 @@ int ftrace_is_dead(void)
 	return ftrace_disabled;
 }
 
+/*
+ * When registering ftrace_ops with IPMODIFY (not direct), 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 on it to enable sharing.
+ *
+ * @ops:     ftrace_ops being registered.
+ *
+ * Returns:
+ *         0 - @ops does have IPMODIFY or @ops itself is DIRECT, no change
+ *             needed;
+ *         1 - @ops has IPMODIFY, hold direct_mutex;
+ *         -EBUSY - currently registered DIRECT ftrace_ops does not support
+ *                  SHARE_IPMODIFY, we need to abort the register.
+ *         -EAGAIN - cannot make changes to currently registered DIRECT
+ *                   ftrace_ops at the moment, but we can retry later. This
+ *                   is needed to avoid potential deadlocks.
+ */
+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) ||
+	    (ops->flags & FTRACE_OPS_FL_DIRECT))
+		return 0;
+
+	mutex_lock(&direct_mutex);

kernel/trace/ftrace.c +29 -0

+				if (ret)
+					goto err_out;
+			}
+		}
+	}
+
+	/*
+	 * Didn't find any overlap with any direct function, 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;
+
+}
+
 /**
  * register_ftrace_function - register a function for profiling
  * @ops:	ops structure that holds the function for profiling.

@@ -7913,17 +8060,27 @@ int ftrace_is_dead(void)
  *       recursive loop.
  */
 int register_ftrace_function(struct ftrace_ops *ops)
+	__releases(&direct_mutex)
 {
+	bool direct_mutex_locked;
 	int ret;
 
 	ftrace_ops_init(ops);
 
+	ret = prepare_direct_functions_for_ipmodify(ops);
+	if (ret < 0)
+		return ret;
+
+	direct_mutex_locked = ret == 1;
+
 	mutex_lock(&ftrace_lock);
 
 	ret = ftrace_startup(ops, 0);
 
 	mutex_unlock(&ftrace_lock);
 
+	if (direct_mutex_locked)
+		mutex_unlock(&direct_mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(register_ftrace_function);

Signature · 1 line

2.30.2

Quoted 5 lines · Song Liu

live patch and BPF trampoline (kfunc/kretfunc in bpftrace) are important features for modern systems. Currently, it is not possible to use live patch and BPF trampoline on the same kernel function at the same time. This is because of the resitriction that only one ftrace_ops with flag FTRACE_OPS_FL_IPMODIFY on the same kernel function.

is it hard to make live patch test? would be great to have selftest for this, or at least sample module that does that, there are already sample modules for direct interface

Quoted 18 lines

BPF trampoline uses direct ftrace_ops, which assumes IPMODIFY. However, not all direct ftrace_ops would overwrite the actual function. This means it is possible to have a non-IPMODIFY direct ftrace_ops to share the same kernel function with an IPMODIFY ftrace_ops.

Introduce FTRACE_OPS_FL_SHARE_IPMODIFY, which allows the direct ftrace_ops to share with IPMODIFY ftrace_ops. With FTRACE_OPS_FL_SHARE_IPMODIFY flag set, the direct ftrace_ops would call the target function picked by the IPMODIFY ftrace_ops.

Comment "IPMODIFY, DIRECT, and SHARE_IPMODIFY" in include/linux/ftrace.h contains more information about how SHARE_IPMODIFY interacts with IPMODIFY and DIRECT flags.

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

Quoted 13 lines

kernel/trace/ftrace.c +13 -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) ||
+	    (ops->flags & FTRACE_OPS_FL_DIRECT))
+		return 0;
+
+	mutex_lock(&direct_mutex);

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

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) {

would it be better to iterate direct_functions hash instead? all the registered direct functions should be there

hm maybe you would not have the 'op' then..

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

Quoted 11 lines

kernel/trace/ftrace.c +10 -0

+				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
+					continue;
+				if (op->flags & FTRACE_OPS_FL_SHARE_IPMODIFY)
+					break;
+				if (ops_references_ip(op, ip)) {
+					found_op = true;
+					break;
+				}
+			} while_for_each_ftrace_op(op);
+			mutex_unlock(&ftrace_lock);

so the 'op' can't go away because it's direct and we hold direct_mutex even though we unlocked ftrace_lock, right?

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

Quoted 8 lines

kernel/trace/ftrace.c +7 -0

+
+			if (found_op) {
+				if (!op->ops_func) {
+					ret = -EBUSY;
+					goto err_out;
+				}
+				ret = op->ops_func(op, FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY);

I did not find call with FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY flag

live patch and BPF trampoline (kfunc/kretfunc in bpftrace) are important features for modern systems. Currently, it is not possible to use live patch and BPF trampoline on the same kernel function at the same time. This is because of the resitriction that only one ftrace_ops with flag FTRACE_OPS_FL_IPMODIFY on the same kernel function.

BPF trampoline uses direct ftrace_ops, which assumes IPMODIFY. However, not all direct ftrace_ops would overwrite the actual function. This means it is possible to have a non-IPMODIFY direct ftrace_ops to share the same kernel function with an IPMODIFY ftrace_ops.

Introduce FTRACE_OPS_FL_SHARE_IPMODIFY, which allows the direct ftrace_ops to share with IPMODIFY ftrace_ops. With FTRACE_OPS_FL_SHARE_IPMODIFY flag set, the direct ftrace_ops would call the target function picked by the IPMODIFY ftrace_ops.

Comment "IPMODIFY, DIRECT, and SHARE_IPMODIFY" in include/linux/ftrace.h contains more information about how SHARE_IPMODIFY interacts with IPMODIFY and DIRECT flags.

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

---
 include/linux/ftrace.h |  74 +++++++++++++++++
 kernel/trace/ftrace.c  | 179 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 242 insertions(+), 11 deletions(-)

include/linux/ftrace.h +0 -0

index 9023bf69f675..bfacf608de9c 100644

include/linux/ftrace.h +10 -0

+ */
+enum ftrace_ops_cmd {
+	FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY,
+	FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY,
+};
+
 #ifdef CONFIG_FUNCTION_TRACER
 
 extern int ftrace_enabled;

@@ -189,6 +201,9 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
  *             ftrace_enabled.
  * DIRECT - Used by the direct ftrace_ops helper for direct functions
  *            (internal ftrace only, should not be used by others)
+ * SHARE_IPMODIFY - For direct ftrace_ops only. Set when the direct function
+ *            is ready to share same kernel function with IPMODIFY function
+ *            (live patch, etc.).
  */
 enum {
 	FTRACE_OPS_FL_ENABLED			= BIT(0),

@@ -209,8 +224,66 @@ enum {
 	FTRACE_OPS_FL_TRACE_ARRAY		= BIT(15),
 	FTRACE_OPS_FL_PERMANENT                 = BIT(16),
 	FTRACE_OPS_FL_DIRECT			= BIT(17),
+	FTRACE_OPS_FL_SHARE_IPMODIFY		= BIT(18),
 };

include/linux/ftrace.h +18 -0

+ *  2. (non-IPMODIFY, and non-SHARE_IPMODIFY) DIRECT ftrace_ops is
+ *     registered first. When the IPMODIFY ftrace_ops is registered later,
+ *     it is necessary to ask the direct ftrace_ops to enable
+ *     SHARE_IPMODIFY support. This is achieved via ftrace_ops->ops_func
+ *     cmd=FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY. For more details on this
+ *     condition, check out prepare_direct_functions_for_ipmodify().
+ */
+
+/*
+ * 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 {

@@ -253,6 +326,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 +0 -0

index 6a419f6bbbf0..868bbc753803 100644

kernel/trace/ftrace.c +1 -0

  * Note that old_hash and new_hash has below meanings
  *  - 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

@@ -1875,6 +1876,7 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 					 struct ftrace_hash *old_hash,
 					 struct ftrace_hash *new_hash)
 {
+	bool is_ipmodify, is_direct, share_ipmodify;
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec, *end = NULL;
 	int in_old, in_new;

@@ -1883,7 +1885,24 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return 0;

kernel/trace/ftrace.c +10 -0

+	 *  #4              0               1                0
+	 *  #5              0               1                1
+	 */
+
+
+	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)   /* combinations #1 */
 		return 0;
 
 	/*

@@ -1893,6 +1912,30 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!new_hash || !old_hash)
 		return -EINVAL;

kernel/trace/ftrace.c +26 -5

+	/*
+	 * Only three combinations of is_ipmodify, is_direct, and
+	 * share_ipmodify for the logic below:
+	 * #2 live patch
+	 * #3 direct with ipmodify
+	 * #4 direct without ipmodify
+	 *
+	 *             is_ipmodify     is_direct     share_ipmodify
+	 *  #2              1               0                0
+	 *  #3              1               1                0
+	 *  #4              0               1                0
+	 *
+	 * Only update/rollback rec->flags for is_ipmodify == 1 (#2 and #3)
+	 */
+
 	/* Update rec->flags */
 	do_for_each_ftrace_rec(pg, rec) {
 

@@ -1906,12 +1949,18 @@ 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)
-				goto rollback;
-			rec->flags |= FTRACE_FL_IPMODIFY;
-		} else /* Removed entry */
+			if (rec->flags & FTRACE_FL_IPMODIFY) {
+				/* cannot have two ipmodify on same rec */
+				if (is_ipmodify)  /* combination #2 and #3 */
+					goto rollback;
+				/* let user enable share_ipmodify and retry */
+				return  -EAGAIN;  /* combination #4 */
+			} else if (is_ipmodify) {
+				rec->flags |= FTRACE_FL_IPMODIFY;
+			}
+		} else if (is_ipmodify) {/* Removed entry */
 			rec->flags &= ~FTRACE_FL_IPMODIFY;
+		}
 	} while_for_each_ftrace_rec();
 
 	return 0;

Quoted 31 lines · On Thu, 2 Jun 2022 12:37:04 -0700 Song Liu <[email protected]> wrote:

live patch and BPF trampoline (kfunc/kretfunc in bpftrace) are important features for modern systems. Currently, it is not possible to use live patch and BPF trampoline on the same kernel function at the same time. This is because of the resitriction that only one ftrace_ops with flag FTRACE_OPS_FL_IPMODIFY on the same kernel function.

BPF trampoline uses direct ftrace_ops, which assumes IPMODIFY. However, not all direct ftrace_ops would overwrite the actual function. This means it is possible to have a non-IPMODIFY direct ftrace_ops to share the same kernel function with an IPMODIFY ftrace_ops.

Introduce FTRACE_OPS_FL_SHARE_IPMODIFY, which allows the direct ftrace_ops to share with IPMODIFY ftrace_ops. With FTRACE_OPS_FL_SHARE_IPMODIFY flag set, the direct ftrace_ops would call the target function picked by the IPMODIFY ftrace_ops.

Comment "IPMODIFY, DIRECT, and SHARE_IPMODIFY" in include/linux/ftrace.h contains more information about how SHARE_IPMODIFY interacts with IPMODIFY and DIRECT flags.

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

---
include/linux/ftrace.h | 74 +++++++++++++++++ kernel/trace/ftrace.c | 179 ++++++++++++++++++++++++++++++++++++++---

2 files changed, 242 insertions(+), 11 deletions(-)

include/linux/ftrace.h +0 -0

index 9023bf69f675..bfacf608de9c 100644

include/linux/ftrace.h static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val · line 106

Quoted 9 lines

include/linux/ftrace.h +6 -0

@@ -98,6 +98,18 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val
 }
 #endif
 
+/*
+ * FTRACE_OPS_CMD_* commands allow the ftrace core logic to request changes
+ * to a ftrace_ops.
+ *
+ * ENABLE_SHARE_IPMODIFY - enable FTRACE_OPS_FL_SHARE_IPMODIFY.
+ * DISABLE_SHARE_IPMODIFY - disable FTRACE_OPS_FL_SHARE_IPMODIFY.

The above comment is basically:

	/* Set x to 1 */
	x = 1;

Probably something like this:

* FTRACE_OPS_CMD_* commands allow the ftrace core logic to request changes
* to a ftrace_ops. Note, the requests may fail.
*
* ENABLE_SHARE_IPMODIFY - Request setting the ftrace ops
* SHARE_IPMODIFY flag.
* DISABLE_SHARE_IPMODIFY - Request disabling the ftrace ops
* SHARE_IPMODIFY flag.

Quoted 25 lines

include/linux/ftrace.h +10 -0

+ */
+enum ftrace_ops_cmd {
+	FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY,
+	FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY,
+};
+
 #ifdef CONFIG_FUNCTION_TRACER
 
 extern int ftrace_enabled;

@@ -189,6 +201,9 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
 * ftrace_enabled.
 * DIRECT - Used by the direct ftrace_ops helper for direct functions
 * (internal ftrace only, should not be used by others)
+ * SHARE_IPMODIFY - For direct ftrace_ops only. Set when the direct function
+ * is ready to share same kernel function with IPMODIFY function
+ * (live patch, etc.).
 */
 enum {
 	FTRACE_OPS_FL_ENABLED			= BIT(0),

@@ -209,8 +224,66 @@ enum {
 	FTRACE_OPS_FL_TRACE_ARRAY		= BIT(15),
 	FTRACE_OPS_FL_PERMANENT = BIT(16),
 	FTRACE_OPS_FL_DIRECT			= BIT(17),
+	FTRACE_OPS_FL_SHARE_IPMODIFY		= BIT(18),
 };

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

Quoted 10 lines

include/linux/ftrace.h +9 -0

 
+/*
+ * IPMODIFY, DIRECT, and SHARE_IPMODIFY.
+ *
+ * ftrace provides IPMODIFY flag for users to replace existing kernel
+ * function with a different version. This is achieved by setting regs->ip.
+ * The top user of IPMODIFY is live patch.
+ *
+ * DIRECT allows user to load custom trampoline on top of ftrace. DIRECT
+ * ftrace does not overwrite regs->ip. Instead, the custom trampoline is

No need to state if DIRECT modifies regs->ip or not. ftrace must assume that it does (more below).

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

Quoted 5 lines

include/linux/ftrace.h +5 -0

+ * saved separately (for example, orig_ax on x86). The top user of DIRECT
+ * is bpf trampoline.
+ *
+ * It is not super rare to have both live patch and bpf trampoline on the
+ * same kernel function. Therefore, it is necessary to allow the two work

"the two to work"

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

Quoted 1 line

include/linux/ftrace.h +1 -0

+ * with each other. Given that IPMODIFY and DIRECT target addressese are

"addresses"

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

Quoted 7 lines

include/linux/ftrace.h +7 -0

+ * saved separately, this is feasible, but we need to be careful.
+ *
+ * The policy between IPMODIFY and DIRECT is:
+ *
+ * 1. Each kernel function can only have one IPMODIFY ftrace_ops;
+ * 2. Each kernel function can only have one DIRECT ftrace_ops;
+ * 3. DIRECT ftrace_ops may have IPMODIFY or not;

I was thinking about this more, and I think by default we should consider all DIRECT ftrace_ops as the same as IPMODIFY. So perhaps the first patch is to just remove the IPMODIFY from direct (as you did) but then make all checks for multiple IPMODIFY also check DIRECT as well.

That is because there's no way that ftrace can verify that a direct trampoline modifies the IP or not. Thus, it must assume that all do.

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

Quoted 10 lines

include/linux/ftrace.h +10 -0

+ * 4. Each kernel function may have one non-DIRECT IPMODIFY ftrace_ops,
+ * and one non-IPMODIFY DIRECT ftrace_ops at the same time. This
+ * requires support from the DIRECT ftrace_ops. Specifically, the
+ * DIRECT trampoline should call the kernel function at regs->ip.
+ * If the DIRECT ftrace_ops supports sharing a function with ftrace_ops
+ * with IPMODIFY, it should set flag SHARE_IPMODIFY.
+ *
+ * Some DIRECT ftrace_ops has an option to enable SHARE_IPMODIFY or not.
+ * Usually, the non-SHARE_IPMODIFY option gives better performance. To take
+ * advantage of this performance benefit, is necessary to only enable

The performance part of this comment should not be in ftrace. It's an implementation detail of the direct trampoline and may not even be accurate with other implementations.

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

Quoted 8 lines

include/linux/ftrace.h +8 -0

+ * SHARE_IPMODIFY only when it is on the same function as an IPMODIFY
+ * ftrace_ops. There are two cases to consider:
+ *
+ * 1. IPMODIFY ftrace_ops is registered first. When the (non-IPMODIFY, and
+ * non-SHARE_IPMODIFY) DIRECT ftrace_ops is registered later,
+ * register_ftrace_direct_multi() returns -EAGAIN. If the user of
+ * the DIRECT ftrace_ops can support SHARE_IPMODIFY, it should enable
+ * SHARE_IPMODIFY and retry.

If this ftrace_ops being registered can support SHARE_IPMODIFY, then it should have the ops_func defined, in which case, why not have it just call that instead of having to return -EAGAIN?

Quoted 33 lines

include/linux/ftrace.h +18 -0

+ * 2. (non-IPMODIFY, and non-SHARE_IPMODIFY) DIRECT ftrace_ops is
+ * registered first. When the IPMODIFY ftrace_ops is registered later,
+ * it is necessary to ask the direct ftrace_ops to enable
+ * SHARE_IPMODIFY support. This is achieved via ftrace_ops->ops_func
+ * cmd=FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY. For more details on this
+ * condition, check out prepare_direct_functions_for_ipmodify().
+ */
+
+/*
+ * 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 {

@@ -253,6 +326,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 +0 -0

index 6a419f6bbbf0..868bbc753803 100644

kernel/trace/ftrace.c static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, · line 1869

Quoted 6 lines

kernel/trace/ftrace.c +2 -1

@@ -1865,7 +1865,8 @@ static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
 /*
 * 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
- * on a ftrace_rec, and -EINVAL if the new_hash tries to trace all recs.
+ * on a ftrace_rec, -EINVAL if the new_hash tries to trace all recs, and
+ * -EAGAIN if the ftrace_ops need to enable SHARE_IPMODIFY.

It should just call the ftrace_ops() with the command to set it. If you want, we could add another CMD enum that can be passed for this case.

Quoted 14 lines

kernel/trace/ftrace.c +1 -0

 * Note that old_hash and new_hash has below meanings
 * - 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

@@ -1875,6 +1876,7 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 					 struct ftrace_hash *old_hash,
 					 struct ftrace_hash *new_hash)
 {
+	bool is_ipmodify, is_direct, share_ipmodify;
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec, *end = NULL;
 	int in_old, in_new;

@@ -1883,7 +1885,24 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return 0;

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

Quoted 10 lines

kernel/trace/ftrace.c +8 -1

 
-	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+	/*
+	 * The following are all the valid combinations of is_ipmodify,
+	 * is_direct, and share_ipmodify
+	 *
+	 * is_ipmodify is_direct share_ipmodify
+	 * #1 0 0 0
+	 * #2 1 0 0
+	 * #3 1 1 0

I still think that DIRECT should automatically be considered IPMODIFY (at least in the view of ftrace, whether or not the direct function modifies the IP).

Quoted 16 lines

kernel/trace/ftrace.c +10 -0

+	 * #4 0 1 0
+	 * #5 0 1 1
+	 */
+
+
+	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) /* combinations #1 */
 		return 0;
 
 	/*

@@ -1893,6 +1912,30 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!new_hash || !old_hash)
 		return -EINVAL;

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

Quoted 10 lines

kernel/trace/ftrace.c +9 -0

 
+	share_ipmodify = ops->flags & FTRACE_OPS_FL_SHARE_IPMODIFY;
+
+	/*
+	 * This ops itself doesn't do ip_modify and it can share a fentry
+	 * with other ops with ipmodify, nothing to do.
+	 */
+	if (!is_ipmodify && share_ipmodify) /* combinations #5 */
+		return 0;
+

Really, if connecting to a function that already has IPMODIFY, then the ops_func() needs to be called, and if the ops supports SHARED_IPMODIFY then it should get set and then continue.

Make sense?

-- Steve

Quoted 42 lines

kernel/trace/ftrace.c +26 -5

+	/*
+	 * Only three combinations of is_ipmodify, is_direct, and
+	 * share_ipmodify for the logic below:
+	 * #2 live patch
+	 * #3 direct with ipmodify
+	 * #4 direct without ipmodify
+	 *
+	 * is_ipmodify is_direct share_ipmodify
+	 * #2 1 0 0
+	 * #3 1 1 0
+	 * #4 0 1 0
+	 *
+	 * Only update/rollback rec->flags for is_ipmodify == 1 (#2 and #3)
+	 */
+
 	/* Update rec->flags */
 	do_for_each_ftrace_rec(pg, rec) {
 

@@ -1906,12 +1949,18 @@ 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)
-				goto rollback;
-			rec->flags |= FTRACE_FL_IPMODIFY;
-		} else /* Removed entry */
+			if (rec->flags & FTRACE_FL_IPMODIFY) {
+				/* cannot have two ipmodify on same rec */
+				if (is_ipmodify) /* combination #2 and #3 */
+					goto rollback;
+				/* let user enable share_ipmodify and retry */
+				return -EAGAIN; /* combination #4 */
+			} else if (is_ipmodify) {
+				rec->flags |= FTRACE_FL_IPMODIFY;
+			}
+		} else if (is_ipmodify) {/* Removed entry */
 			rec->flags &= ~FTRACE_FL_IPMODIFY;
+		}
 	} while_for_each_ftrace_rec();
 
 	return 0;

I think there is one more problem here. If we force all direct trampoline set IPMODIFY, and remove the SHARE_IPMODIFY flag. It may cause confusion and/or extra work here (__ftrace_hash_update_ipmodify).

Say __ftrace_hash_update_ipmodify() tries to attach an ops with IPMODIFY, and found the rec already has IPMODIFY. At this point, we have to iterate all ftrace ops (do_for_each_ftrace_op) to confirm whether the IPMODIFY is from

1) a direct ops that can share IPMODIFY, or
2) a direct ops that cannot share IPMODIFY, or
3) another non-direct ops with IPMODIFY.

For the 1), this attach works, while for 2) and 3), the attach doesn't work.

OTOH, with current version (v2), we trust the direct ops to set or clear IPMODIFY. rec with IPMODIFY makes it clear that it cannot share with another ops with IPMODIFY. Then we don't have to iterate over all ftrace ops here.

Does this make sense? Did I miss some better solutions?

Quoted 3 lines · On Fri, 15 Jul 2022 00:13:51 +0000 Song Liu <[email protected]> wrote:

I think there is one more problem here. If we force all direct trampoline set IPMODIFY, and remove the SHARE_IPMODIFY flag. It may cause confusion and/or extra work here (__ftrace_hash_update_ipmodify).

I'm saying that the caller (BPF) does not need to set IPMODIFY. Just change the ftrace.c to treat IPMODIFY the same as direct. In fact, the two should be mutually exclusive (from a ftrace point of view). That is, if DIRECT is set, then IPMODIFY must not be.

Again, ftrace will take care of the accounting of if a rec has both IPMODIFY and DIRECT on it.

Quoted 1 line

Quoted 4 lines

Say __ftrace_hash_update_ipmodify() tries to attach an ops with IPMODIFY, and found the rec already has IPMODIFY. At this point, we have to iterate all ftrace ops (do_for_each_ftrace_op) to confirm whether the IPMODIFY is from

What I'm suggesting is that a DIRECT ops will never set IPMODIFY.

Like
I mentioned before, ftrace doesn't care if the direct trampoline modifies IP or not. All ftrace will do is ask the owner of the direct ops if it is safe to have an IP modify attached to it or not. And at the same time will tell the direct ops owner that it is adding an IPMODIFY ops such that it can take the appropriate actions.

In other words, IPMODIFY on the rec means that it can not attach anything else with an IPMODIFY on it.

The direct ops should only set the DIRECT flag. If an IPMODIFY ops is being added, if it already has an IPMODIFY then it will fail right there.

If direct is set, then a loop for the direct ops will be done and the callback is made. If the direct ops is OK with the IPMODIFY then it will adjust itself to work with the IPMODIFY and the IPMODIFY will continue to be added (and then set the rec IPMODIFY flag).

Quoted 5 lines

1) a direct ops that can share IPMODIFY, or
2) a direct ops that cannot share IPMODIFY, or
3) another non-direct ops with IPMODIFY.

Quoted 5 lines

For the 1), this attach works, while for 2) and 3), the attach doesn't work.

OTOH, with current version (v2), we trust the direct ops to set or clear IPMODIFY. rec with IPMODIFY makes it clear that it cannot share with another ops with IPMODIFY. Then we don't have to iterate over all ftrace ops here.

The only time an iterate should be done is if rec->flags is DIRECT and !IPMODIFY.

Quoted 3 lines

Does this make sense? Did I miss some better solutions?

Did I fill in the holes? ;-)

Quoted 1 line

What I'm suggesting is that a DIRECT ops will never set IPMODIFY.

Aha, this the point I misunderstood. I thought DIRECT ops would always set IPMODIFY (as it does now).

Quoted 4 lines · On Fri, 15 Jul 2022 02:04:33 +0000 Song Liu <[email protected]> wrote:

Quoted 1 line

What I'm suggesting is that a DIRECT ops will never set IPMODIFY.

Aha, this the point I misunderstood. I thought DIRECT ops would always set IPMODIFY (as it does now).

My fault. I was probably not being clear when I was suggesting that DIRECT should *act* like an IPMODIFY, but never explicitly stated that it should not set the IPMODIFY flag.

The only reason it does today was to make it easy to act like an IPMODIFY (because it set the flag). But I'm now suggesting to get rid of that and just make DIRECT act like an IPMDOFIY as there can only be one of them on a function, but now we have some cases where DIRECT can work with IPMODIFY via the callbacks.

Quoted 17 lines · Steven Rostedt

Quoted 4 lines · On Fri, 15 Jul 2022 02:04:33 +0000 Song Liu <[email protected]> wrote:

Quoted 1 line

What I'm suggesting is that a DIRECT ops will never set IPMODIFY.

Aha, this the point I misunderstood. I thought DIRECT ops would always set IPMODIFY (as it does now).

My fault. I was probably not being clear when I was suggesting that DIRECT should *act* like an IPMODIFY, but never explicitly stated that it should not set the IPMODIFY flag.

Quoted 5 lines

The only reason it does today was to make it easy to act like an IPMODIFY (because it set the flag). But I'm now suggesting to get rid of that and just make DIRECT act like an IPMDOFIY as there can only be one of them on a function, but now we have some cases where DIRECT can work with IPMODIFY via the callbacks.

Thanks for the clarification. I think we are finally on the same page on this. :)

A quick update and ask for feedback/clarification.

Based on my understanding, you recommended calling ops_func() from __ftrace_hash_update_ipmodify() and in ops_func() the direct trampoline may make changes to the trampoline. Did I get this right?

I am going towards this direction, but hit some issue. Specifically, in __ftrace_hash_update_ipmodify(), ftrace_lock is already locked, so the direct trampoline cannot easily make changes with modify_ftrace_direct_multi(), which locks both direct_mutex and ftrace_mutex.

One solution would be have no-lock version of all the functions called by modify_ftrace_direct_multi(), but that's a lot of functions and the code will be pretty ugly. The alternative would be the logic in v2:
__ftrace_hash_update_ipmodify() returns -EAGAIN, and we make changes to the direct trampoline in other places:

1) if DIRECT ops attached first, the trampoline is updated in prepare_direct_functions_for_ipmodify(), see 3/5 of v2;

2) if IPMODIFY ops attached first, the trampoline is updated in bpf_trampoline_update(), see "goto again" path in 5/5 of v2.

Overall, I think this way is still cleaner. What do you think about this?

Quoted 22 lines · On Fri, 15 Jul 2022 17:42:55 +0000 Song Liu <[email protected]> wrote:

A quick update and ask for feedback/clarification.

Based on my understanding, you recommended calling ops_func() from __ftrace_hash_update_ipmodify() and in ops_func() the direct trampoline may make changes to the trampoline. Did I get this right?

I am going towards this direction, but hit some issue. Specifically, in __ftrace_hash_update_ipmodify(), ftrace_lock is already locked, so the direct trampoline cannot easily make changes with modify_ftrace_direct_multi(), which locks both direct_mutex and ftrace_mutex.

One solution would be have no-lock version of all the functions called by modify_ftrace_direct_multi(), but that's a lot of functions and the code will be pretty ugly. The alternative would be the logic in v2:
__ftrace_hash_update_ipmodify() returns -EAGAIN, and we make changes to the direct trampoline in other places:

1) if DIRECT ops attached first, the trampoline is updated in prepare_direct_functions_for_ipmodify(), see 3/5 of v2;

Quoted 4 lines

2) if IPMODIFY ops attached first, the trampoline is updated in bpf_trampoline_update(), see "goto again" path in 5/5 of v2.

Overall, I think this way is still cleaner. What do you think about this?

What about if we release the lock when doing the callback?

Then we just need to make sure things are the same after reacquiring the lock, and if they are different, we release the lock again and do the callback with the new update. Wash, rinse, repeat, until the state is the same before and after the callback with locks acquired?

This is a common way to handle callbacks that need to do something that takes the lock held before doing a callback.

The reason I say this, is because the more we can keep the accounting inside of ftrace the better.

Wouldn't this need to be done anyway if BPF was first and live kernel patching needed the update? An -EAGAIN would not suffice.

Quoted 2 lines

What about if we release the lock when doing the callback?

We can probably unlock ftrace_lock here. But we may break locking order with direct mutex (see below).

Quoted 6 lines

Then we just need to make sure things are the same after reacquiring the lock, and if they are different, we release the lock again and do the callback with the new update. Wash, rinse, repeat, until the state is the same before and after the callback with locks acquired?

Personally, I would like to avoid wash-rinse-repeat here.

Quoted 10 lines

This is a common way to handle callbacks that need to do something that takes the lock held before doing a callback.

The reason I say this, is because the more we can keep the accounting inside of ftrace the better.

Wouldn't this need to be done anyway if BPF was first and live kernel patching needed the update? An -EAGAIN would not suffice.

prepare_direct_functions_for_ipmodify handles BPF-first-livepatch-later case. The benefit of prepare_direct_functions_for_ipmodify() is that it holds direct_mutex before ftrace_lock, and keeps holding it if necessary. This is enough to make sure we don't need the wash-rinse-repeat.

OTOH, if we wait until __ftrace_hash_update_ipmodify(), we already hold ftrace_lock, but not direct_mutex. To make changes to bpf trampoline, we have to unlock ftrace_lock and lock direct_mutex to avoid deadlock. However, this means we will need the wash-rinse-repeat.

For livepatch-first-BPF-later case, we can probably handle this in __ftrace_hash_update_ipmodify(), since we hold both direct_mutex and ftrace_lock. We can unlock ftrace_lock and update the BPF trampoline. It is safe against changes to direct ops, because we are still holding direct_mutex. But, is this safe against another IPMODIFY ops? I am not sure yet... Also, this is pretty weird because, we are updating a direct trampoline before we finish registering it for the first time. IOW, we are calling modify_ftrace_direct_multi_nolock for the same trampoline before register_ftrace_direct_multi() returns.

The approach in v2 propagates the -EAGAIN to BPF side, so these are two independent calls of register_ftrace_direct_multi(). This does require some protocol between ftrace core and its user, but I still think this is a cleaner approach.

Quoted 1 line · On Fri, 15 Jul 2022 19:49:00 +0000 Song Liu <[email protected]> wrote:

Quoted 1 line

Quoted 4 lines

Quoted 1 line

What about if we release the lock when doing the callback?

We can probably unlock ftrace_lock here. But we may break locking order with direct mutex (see below).

You're talking about the multi registering case, right?

Quoted 9 lines

Quoted 7 lines

Then we just need to make sure things are the same after reacquiring the lock, and if they are different, we release the lock again and do the callback with the new update. Wash, rinse, repeat, until the state is the same before and after the callback with locks acquired?

Personally, I would like to avoid wash-rinse-repeat here.

But it's common to do. Keeps your hair cleaner that way ;-)

Quoted 20 lines

Quoted 10 lines

This is a common way to handle callbacks that need to do something that takes the lock held before doing a callback.

The reason I say this, is because the more we can keep the accounting inside of ftrace the better.

Wouldn't this need to be done anyway if BPF was first and live kernel patching needed the update? An -EAGAIN would not suffice.

prepare_direct_functions_for_ipmodify handles BPF-first-livepatch-later case. The benefit of prepare_direct_functions_for_ipmodify() is that it holds direct_mutex before ftrace_lock, and keeps holding it if necessary. This is enough to make sure we don't need the wash-rinse-repeat.

OTOH, if we wait until __ftrace_hash_update_ipmodify(), we already hold ftrace_lock, but not direct_mutex. To make changes to bpf trampoline, we have to unlock ftrace_lock and lock direct_mutex to avoid deadlock. However, this means we will need the wash-rinse-repeat.

Quoted 12 lines

For livepatch-first-BPF-later case, we can probably handle this in __ftrace_hash_update_ipmodify(), since we hold both direct_mutex and ftrace_lock. We can unlock ftrace_lock and update the BPF trampoline. It is safe against changes to direct ops, because we are still holding direct_mutex. But, is this safe against another IPMODIFY ops? I am not sure yet... Also, this is pretty weird because, we are updating a direct trampoline before we finish registering it for the first time. IOW, we are calling modify_ftrace_direct_multi_nolock for the same trampoline before register_ftrace_direct_multi() returns.

Quoted 4 lines

The approach in v2 propagates the -EAGAIN to BPF side, so these are two independent calls of register_ftrace_direct_multi(). This does require some protocol between ftrace core and its user, but I still think this is a cleaner approach.

The issue I have with this approach is it couples BPF and ftrace a bit too much.

But there is a way with my approach you can still do your approach. That is, have ops_func() return zero if everything is fine, and otherwise returns a negative value. Then have the register function fail and return whatever value that gets returned by the ops_func()

Then have the bpf ops_func() check (does this direct caller handle IPMODIFY? if yes, return 0, else return -EAGAIN). Then the registering of ftrace fails with your -EAGAIN, and then you can change the direct trampoline to handle IPMODIFY and try again. This time when ops_func() is called, it sees that the direct trampoline can handle the IPMODIFY and returns 0.

Basically, it's a way to still implement my suggestion, but let BPF decide to use -EAGAIN to try again. And then BPF and ftrace don't need to have these special flags to change the behavior of each other.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Wouldn't this need to be done anyway if BPF was first and live kernel patching needed the update? An -EAGAIN would not suffice.

prepare_direct_functions_for_ipmodify handles BPF-first-livepatch-later case. The benefit of prepare_direct_functions_for_ipmodify() is that it holds direct_mutex before ftrace_lock, and keeps holding it if necessary. This is enough to make sure we don't need the wash-rinse-repeat.

Quoted 4 lines

Quoted 4 lines

OTOH, if we wait until __ftrace_hash_update_ipmodify(), we already hold ftrace_lock, but not direct_mutex. To make changes to bpf trampoline, we have to unlock ftrace_lock and lock direct_mutex to avoid deadlock. However, this means we will need the wash-rinse-repeat.

What do you think about the prepare_direct_functions_for_ipmodify() approach? If this is not ideal, maybe we can simplify it so that it only holds direct_mutex (when necessary). The benefit is that we are sure direct_mutex is already held in __ftrace_hash_update_ipmodify(). However, I think it is not safe to unlock ftrace_lock in __ftrace_hash_update_ipmodify(). We can get parallel do_for_each_ftrace_rec(), which is dangerous, no?

int register_ftrace_function(struct ftrace_ops *ops)

(quoted diff) +2 -0

+	__releases(&direct_mutex)
 {
+	bool direct_mutex_locked;

(quoted diff) +2 -0

 	mutex_lock(&ftrace_lock);
 
 	ret = ftrace_startup(ops, 0);
 
 	mutex_unlock(&ftrace_lock);
 
+	if (direct_mutex_locked)
+		mutex_unlock(&direct_mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(register_ftrace_function);

Quoted 13 lines · On Fri, 15 Jul 2022 20:21:49 +0000 Song Liu <[email protected]> wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Wouldn't this need to be done anyway if BPF was first and live kernel patching needed the update? An -EAGAIN would not suffice.

prepare_direct_functions_for_ipmodify handles BPF-first-livepatch-later case. The benefit of prepare_direct_functions_for_ipmodify() is that it holds direct_mutex before ftrace_lock, and keeps holding it if necessary. This is enough to make sure we don't need the wash-rinse-repeat.

Quoted 4 lines

Quoted 4 lines

OTOH, if we wait until __ftrace_hash_update_ipmodify(), we already hold ftrace_lock, but not direct_mutex. To make changes to bpf trampoline, we have to unlock ftrace_lock and lock direct_mutex to avoid deadlock. However, this means we will need the wash-rinse-repeat.

Quoted 6 lines

What do you think about the prepare_direct_functions_for_ipmodify() approach? If this is not ideal, maybe we can simplify it so that it only holds direct_mutex (when necessary). The benefit is that we are sure direct_mutex is already held in __ftrace_hash_update_ipmodify(). However, I think it is not safe to unlock ftrace_lock in __ftrace_hash_update_ipmodify(). We can get parallel do_for_each_ftrace_rec(), which is dangerous, no?

I'm fine with it. But one nit on the logic:

Quoted 8 lines

int register_ftrace_function(struct ftrace_ops *ops)

(quoted diff) +1 -0

+	__releases(&direct_mutex)

{

(quoted diff) +1 -0

+	bool direct_mutex_locked;

	int ret;

	ftrace_ops_init(ops);

Review

Quoted 6 lines

(quoted diff) +6 -0

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

Please make the above:

	if (ret < 0)
		return ret;
	else if (ret == 1)
		direct_mutex_locked = true;

It's much easier to read that way.

Quoted 4 lines

Review

Quoted 4 lines

int register_ftrace_function(struct ftrace_ops *ops)

(quoted diff) +1 -0

+	__releases(&direct_mutex)

{

(quoted diff) +1 -0

+	bool direct_mutex_locked;

Quoted 13 lines

Review

Quoted 10 lines

	int ret;

	ftrace_ops_init(ops);

(quoted diff) +6 -0

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

Please make the above:

Quoted 6 lines

	if (ret < 0)
		return ret;
	else if (ret == 1)
		direct_mutex_locked = true;

It's much easier to read that way.

Thanks for the clarification!

kernel/bpf/trampoline.c +7 -0

@@ -27,6 +27,44 @@ static struct hlist_head trampoline_table[TRAMPOLINE_TABLE_SIZE];
 /* serializes access to trampoline_table */
 static DEFINE_MUTEX(trampoline_mutex);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex);
+
+static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
+{
+	struct bpf_trampoline *tr = ops->private;
+	int ret;

kernel/bpf/trampoline.c +36 -2

+		return -EAGAIN;
+
+	switch (cmd) {
+	case FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY:
+		tr->indirect_call = true;
+		ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
+		break;
+	case FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY:
+		tr->indirect_call = false;
+		tr->fops->flags &= ~FTRACE_OPS_FL_SHARE_IPMODIFY;
+		ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	};
+	mutex_unlock(&tr->mutex);
+	return ret;
+}
+#endif
+
 
 
 

@@ -330,7 +387,7 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx)
 	return ERR_PTR(err);
 }
 
-static int bpf_trampoline_update(struct bpf_trampoline *tr)
+static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex)
 {
 	struct bpf_tramp_image *im;
 	struct bpf_tramp_links *tlinks;

@@ -363,20 +420,45 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	if (ip_arg)
 		flags |= BPF_TRAMP_F_IP_ARG;
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+again:
+	if (tr->indirect_call)
+		flags |= BPF_TRAMP_F_ORIG_STACK;
+#endif
+
 	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
 					  &tr->func.model, flags, tlinks,
 					  tr->func.addr);
 	if (err < 0)
 		goto out;
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+	if (tr->indirect_call)
+		tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
+#endif
+
 	WARN_ON(tr->cur_image && tr->selector == 0);
 	WARN_ON(!tr->cur_image && tr->selector);
 	if (tr->cur_image)
 		/* progs already running at this address */
-		err = modify_fentry(tr, tr->cur_image->image, im->image);
+		err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex);
 	else
 		/* first time registering */
 		err = register_fentry(tr, im->image);
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS

kernel/bpf/trampoline.c +5 -2

+	}
+#endif
 	if (err)
 		goto out;
 	if (tr->cur_image)

@@ -460,7 +542,7 @@ int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline
 
 	hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
 	tr->progs_cnt[kind]++;
-	err = bpf_trampoline_update(tr);
+	err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
 	if (err) {
 		hlist_del_init(&link->tramp_hlist);
 		tr->progs_cnt[kind]--;

@@ -487,7 +569,7 @@ int bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampolin
 	}
 	hlist_del_init(&link->tramp_hlist);
 	tr->progs_cnt[kind]--;
-	err = bpf_trampoline_update(tr);
+	err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
 out:
 	mutex_unlock(&tr->mutex);
 	return err;

@@ -535,6 +617,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
 	 * multiple rcu callbacks.
 	 */
 	hlist_del(&tr->hlist);
+	kfree(tr->fops);
 	kfree(tr);
 out:
 	mutex_unlock(&trampoline_mutex);

Quoted 18 lines · On Thu, 2 Jun 2022 12:37:06 -0700 Song Liu <[email protected]> wrote:

kernel/bpf/trampoline.c +12 -0

@@ -27,6 +27,44 @@ static struct hlist_head trampoline_table[TRAMPOLINE_TABLE_SIZE];
 /* serializes access to trampoline_table */
 static DEFINE_MUTEX(trampoline_mutex);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex);
+
+static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
+{
+	struct bpf_trampoline *tr = ops->private;
+	int ret;
+
+	/*
+	 * The normal locking order is
+	 *    tr->mutex => direct_mutex (ftrace.c) => ftrace_lock (ftrace.c)
+	 *

kernel/bpf/trampoline.c static struct hlist_head trampoline_table[TRAMPOLINE_TABLE_SIZE]; · line 46

Quoted 5 lines

kernel/bpf/trampoline.c +5 -0

+	 * This is called from prepare_direct_functions_for_ipmodify, with
+	 * direct_mutex locked. Use mutex_trylock() to avoid dead lock.
+	 * Also, bpf_trampoline_update here should not lock direct_mutex.
+	 */
+	if (!mutex_trylock(&tr->mutex))

Can you comment here that returning -EAGAIN will not cause this to repeat. That it will change things where the next try will not return -EGAIN?

Quoted 66 lines

kernel/bpf/trampoline.c +36 -2

+		return -EAGAIN;
+
+	switch (cmd) {
+	case FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY:
+		tr->indirect_call = true;
+		ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
+		break;
+	case FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY:
+		tr->indirect_call = false;
+		tr->fops->flags &= ~FTRACE_OPS_FL_SHARE_IPMODIFY;
+		ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	};
+	mutex_unlock(&tr->mutex);
+	return ret;
+}
+#endif
+
 
 
 

@@ -330,7 +387,7 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx)
 	return ERR_PTR(err);
 }
 
-static int bpf_trampoline_update(struct bpf_trampoline *tr)
+static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex)
 {
 	struct bpf_tramp_image *im;
 	struct bpf_tramp_links *tlinks;

@@ -363,20 +420,45 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	if (ip_arg)
 		flags |= BPF_TRAMP_F_IP_ARG;
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+again:
+	if (tr->indirect_call)
+		flags |= BPF_TRAMP_F_ORIG_STACK;
+#endif
+
 	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
 					  &tr->func.model, flags, tlinks,
 					  tr->func.addr);
 	if (err < 0)
 		goto out;
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+	if (tr->indirect_call)
+		tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
+#endif
+
 	WARN_ON(tr->cur_image && tr->selector == 0);
 	WARN_ON(!tr->cur_image && tr->selector);
 	if (tr->cur_image)
 		/* progs already running at this address */
-		err = modify_fentry(tr, tr->cur_image->image, im->image);
+		err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex);
 	else
 		/* first time registering */
 		err = register_fentry(tr, im->image);
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS

kernel/bpf/trampoline.c static int bpf_trampoline_update(struct bpf_trampoline *tr) · line 459

Quoted 10 lines

kernel/bpf/trampoline.c +10 -0

+	if (err == -EAGAIN) {
+		if (WARN_ON_ONCE(tr->indirect_call))
+			goto out;
+		/* should only retry on the first register */
+		if (WARN_ON_ONCE(tr->cur_image))
+			goto out;
+		tr->indirect_call = true;
+		tr->fops->func = NULL;
+		tr->fops->trampoline = 0;
+		goto again;

I'm assuming that the above will prevent a return of -EAGAIN again. As if it can, then this could turn into a dead lock.

Can you please comment that?

Quoted 2 lines

Can you comment here that returning -EAGAIN will not cause this to repeat. That it will change things where the next try will not return -EGAIN?

Hmm.. this is not the guarantee here. This conflict is a real race condition that an IPMODIFY function (i.e. livepatch) is being registered at the same time when something else, for example bpftrace, is updating the BPF trampoline.

This EAGAIN will propagate to the user of the IPMODIFY function (i.e. livepatch), and we need to retry there. In the case of livepatch, the retry is initiated from user space.

Quoted 10 lines · On Wed, 6 Jul 2022 21:37:52 +0000 Song Liu <[email protected]> wrote:

Quoted 2 lines

Can you comment here that returning -EAGAIN will not cause this to repeat. That it will change things where the next try will not return -EGAIN?

Hmm.. this is not the guarantee here. This conflict is a real race condition that an IPMODIFY function (i.e. livepatch) is being registered at the same time when something else, for example bpftrace, is updating the BPF trampoline.

This EAGAIN will propagate to the user of the IPMODIFY function (i.e. livepatch), and we need to retry there. In the case of livepatch, the retry is initiated from user space.

We need to be careful here then. If there's a userspace application that runs at real-time and does a:

	do {
		errno = 0;
		regsiter_bpf();
	} while (errno != -EAGAIN);

it could in theory preempt the owner of the lock and never make any progress.

Quoted 10 lines · On Wed, 6 Jul 2022 21:37:52 +0000 Song Liu <[email protected]> wrote:

Quoted 2 lines

Can you comment here that returning -EAGAIN will not cause this to repeat. That it will change things where the next try will not return -EGAIN?

Hmm.. this is not the guarantee here. This conflict is a real race condition that an IPMODIFY function (i.e. livepatch) is being registered at the same time when something else, for example bpftrace, is updating the BPF trampoline.

This EAGAIN will propagate to the user of the IPMODIFY function (i.e. livepatch), and we need to retry there. In the case of livepatch, the retry is initiated from user space.

We need to be careful here then. If there's a userspace application that runs at real-time and does a:

	do {
		errno = 0;
		regsiter_bpf();
	} while (errno != -EAGAIN);

Quoted 16 lines · Steven Rostedt

Quoted 10 lines · On Wed, 6 Jul 2022 21:37:52 +0000 Song Liu <[email protected]> wrote:

Quoted 2 lines

Can you comment here that returning -EAGAIN will not cause this to repeat. That it will change things where the next try will not return -EGAIN?

Hmm.. this is not the guarantee here. This conflict is a real race condition that an IPMODIFY function (i.e. livepatch) is being registered at the same time when something else, for example bpftrace, is updating the BPF trampoline.

This EAGAIN will propagate to the user of the IPMODIFY function (i.e. livepatch), and we need to retry there. In the case of livepatch, the retry is initiated from user space.

Quoted 7 lines

We need to be careful here then. If there's a userspace application that runs at real-time and does a:

	do {
		errno = 0;
		regsiter_bpf();
	} while (errno != -EAGAIN);  

Actually, do you mean:

	do {
		errno = 0;
		regsiter_bpf();
	} while (errno == -EAGAIN);

(== -EAGAIN) here?

In this specific race condition, register_bpf() will succeed, as it already got tr->mutex. But the IPMODIFY (livepatch) side will fail and retry.

Since both livepatch and bpf trampoline changes are rare operations, I think the chance of the race condition is low enough.

Does this make sense?

Quoted 24 lines · On Wed, 6 Jul 2022 22:15:47 +0000 Song Liu <[email protected]> wrote:

Quoted 16 lines · Steven Rostedt

Quoted 10 lines · On Wed, 6 Jul 2022 21:37:52 +0000 Song Liu <[email protected]> wrote:

Quoted 2 lines

Can you comment here that returning -EAGAIN will not cause this to repeat. That it will change things where the next try will not return -EGAIN?

Hmm.. this is not the guarantee here. This conflict is a real race condition that an IPMODIFY function (i.e. livepatch) is being registered at the same time when something else, for example bpftrace, is updating the BPF trampoline.

This EAGAIN will propagate to the user of the IPMODIFY function (i.e. livepatch), and we need to retry there. In the case of livepatch, the retry is initiated from user space.

Quoted 7 lines

We need to be careful here then. If there's a userspace application that runs at real-time and does a:

	do {
		errno = 0;
		regsiter_bpf();
	} while (errno != -EAGAIN);  

Quoted 8 lines

Actually, do you mean:

	do {
		errno = 0;
		regsiter_bpf();
	} while (errno == -EAGAIN);

(== -EAGAIN) here?

Yeah, of course.

Quoted 3 lines

In this specific race condition, register_bpf() will succeed, as it already got tr->mutex. But the IPMODIFY (livepatch) side will fail and retry.

What else takes the tr->mutex ?

If it preempts anything else taking that mutex, when this runs, then it needs to be careful.

You said this can happen when the live patch came first. This isn't racing against live patch, it's racing against anything that takes the tr->mutex and then adds a bpf trampoline to a location that has a live patch.

Quoted 7 lines

Since both livepatch and bpf trampoline changes are rare operations, I think the chance of the race condition is low enough.

Does this make sense?

It's low, and if it is also a privileged operation then there's less to be concern about.

As if it is not, then we could have a way to deadlock the system. I'm more concerned that this will lead to a CVE than it just happening randomly. In other words, it only takes something that can run at a real-time priority to connect to a live patch location, and something that runs at a low priority to take a tr->mutex. If an attacker has both, then it can pin both to a CPU and then cause the deadlock to the system.

One hack to fix this is to add a msleep(1) in the failed case of the trylock. This will at least give the owner of the lock a millisecond to release it. This was what the RT patch use to do with spin_trylock() that was converted to a mutex (and we worked hard to remove all of them).

Quoted 4 lines

Quoted 2 lines

In this specific race condition, register_bpf() will succeed, as it already got tr->mutex. But the IPMODIFY (livepatch) side will fail and retry.

What else takes the tr->mutex ?

tr->mutex is the local mutex for a single BPF trampoline, we only need to take it when we make changes to the trampoline (add/remove fentry/fexit programs).

Quoted 4 lines

If it preempts anything else taking that mutex, when this runs, then it needs to be careful.

Quoted 3 lines

You said this can happen when the live patch came first. This isn't racing against live patch, it's racing against anything that takes the tr->mutex and then adds a bpf trampoline to a location that has a live patch.

There are a few scenarios here:
1. Live patch is already applied, then a BPF trampoline is being registered to the same function. In bpf_trampoline_update(), register_fentry returns -EAGAIN, and this will be resolved.

2. BPF trampoline is already registered, then a live patch is being applied to the same function. The live patch code need to ask the bpf trampoline to prepare the trampoline before live patch. This is done by calling bpf_tramp_ftrace_ops_func.

2.1 If nothing else is modifying the trampoline at the same time, bpf_tramp_ftrace_ops_func will succeed.

2.2 In rare cases, if something else is holding tr->mutex to make changes to the trampoline (add/remove fentry functions, etc.), mutex_trylock in bpf_tramp_ftrace_ops_func will fail, and live patch will fail. However, the change to BPF trampoline will still succeed. It is common for live patch to retry, so we just need to try live patch again when no one is making changes to the BPF trampoline in parallel.

Quoted 5 lines

Quoted 5 lines

Since both livepatch and bpf trampoline changes are rare operations, I think the chance of the race condition is low enough.

Quoted 6 lines

Quoted 3 lines

Does this make sense?

It's low, and if it is also a privileged operation then there's less to be concern about.

Both live patch and BPF trampoline are privileged operations.

Quoted 8 lines

As if it is not, then we could have a way to deadlock the system. I'm more concerned that this will lead to a CVE than it just happening randomly. In other words, it only takes something that can run at a real-time priority to connect to a live patch location, and something that runs at a low priority to take a tr->mutex. If an attacker has both, then it can pin both to a CPU and then cause the deadlock to the system.

Quoted 4 lines

One hack to fix this is to add a msleep(1) in the failed case of the trylock. This will at least give the owner of the lock a millisecond to release it. This was what the RT patch use to do with spin_trylock() that was converted to a mutex (and we worked hard to remove all of them).

The fix is really simple. But I still think we don't need it. We only hit the trylock case for something with IPMODIFY. The non-privileged user should not be able to do that, right?

Quoted 16 lines · On Thu, 7 Jul 2022 00:19:07 +0000 Song Liu <[email protected]> wrote:

Quoted 4 lines

Quoted 2 lines

In this specific race condition, register_bpf() will succeed, as it already got tr->mutex. But the IPMODIFY (livepatch) side will fail and retry.

What else takes the tr->mutex ?

tr->mutex is the local mutex for a single BPF trampoline, we only need to take it when we make changes to the trampoline (add/remove fentry/fexit programs).

Quoted 7 lines

If it preempts anything else taking that mutex, when this runs, then it needs to be careful.

You said this can happen when the live patch came first. This isn't racing against live patch, it's racing against anything that takes the tr->mutex and then adds a bpf trampoline to a location that has a live patch.

Quoted 4 lines

There are a few scenarios here:
1. Live patch is already applied, then a BPF trampoline is being registered to the same function. In bpf_trampoline_update(), register_fentry returns -EAGAIN, and this will be resolved.

Where will it be resolved?

Quoted 10 lines

2. BPF trampoline is already registered, then a live patch is being applied to the same function. The live patch code need to ask the bpf trampoline to prepare the trampoline before live patch. This is done by calling bpf_tramp_ftrace_ops_func.

2.1 If nothing else is modifying the trampoline at the same time, bpf_tramp_ftrace_ops_func will succeed.

Quoted 6 lines

2.2 In rare cases, if something else is holding tr->mutex to make changes to the trampoline (add/remove fentry functions, etc.), mutex_trylock in bpf_tramp_ftrace_ops_func will fail, and live patch will fail. However, the change to BPF trampoline will still succeed. It is common for live patch to retry, so we just need to try live patch again when no one is making changes to the BPF trampoline in parallel.

If the live patch is going to try again, and the task doing the live patch is SCHED_FIFO, and the task holding the tr->mutex is SCHED_OTHER (or just a lower priority), then there is a chance that the live patch task preempted the tr->mutex owner, and let's say the tr->mutex owner is pinned to the CPU (by the user or whatever), then because the live patch task is in a loop trying to take that mutex, it will never let the owner continue.

Yes, this is a real scenario with trylock on mutexes. We hit it all the time in RT.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Since both livepatch and bpf trampoline changes are rare operations, I think the chance of the race condition is low enough.

A low race condition in a world that does this a billion times a day, ends up being not so rare.

I like to say, "I live in a world where the unlikely is very much likely!"

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Does this make sense?

It's low, and if it is also a privileged operation then there's less to be concern about.

Both live patch and BPF trampoline are privileged operations.

This makes the issue less of an issue, but if there's an application that does this with setuid or something, there's a chance that it can be used by an attacker as well.

Quoted 8 lines

Quoted 8 lines

As if it is not, then we could have a way to deadlock the system. I'm more concerned that this will lead to a CVE than it just happening randomly. In other words, it only takes something that can run at a real-time priority to connect to a live patch location, and something that runs at a low priority to take a tr->mutex. If an attacker has both, then it can pin both to a CPU and then cause the deadlock to the system.

Quoted 8 lines

Quoted 4 lines

One hack to fix this is to add a msleep(1) in the failed case of the trylock. This will at least give the owner of the lock a millisecond to release it. This was what the RT patch use to do with spin_trylock() that was converted to a mutex (and we worked hard to remove all of them).

The fix is really simple. But I still think we don't need it. We only hit the trylock case for something with IPMODIFY. The non-privileged user should not be able to do that, right?

For now, perhaps. But what useful applications are there going to be in the future that performs these privileged operations on behalf of a non-privileged user?

In other words, if we can fix it now, we should, and avoid debugging this issue 5 years from now where it may take months to figure out.

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 non-DIRECT IPMODIFY ftrace_ops and one non-IPMODIFY DIRECT ftrace_ops on the same kernel function at the same time. Please see 3/5 for more details on this.

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.

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

Song Liu (4):
ftrace: allow customized flags for ftrace_direct_multi ftrace_ops
ftrace: add modify_ftrace_direct_multi_nolock
ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY
bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY

arch/x86/net/bpf_jit_comp.c |  13 +-
include/linux/bpf.h         |   8 ++
include/linux/ftrace.h      |  79 +++++++++++
kernel/bpf/trampoline.c     | 109 +++++++++++++--
kernel/trace/ftrace.c       | 269 +++++++++++++++++++++++++++++++-----
5 files changed, 424 insertions(+), 54 deletions(-)

--
2.30.2

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 non-DIRECT IPMODIFY ftrace_ops and one non-IPMODIFY DIRECT ftrace_ops on the same kernel function at the same time. Please see 3/5 for more details on this.

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.

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

Song Liu (4):
ftrace: allow customized flags for ftrace_direct_multi ftrace_ops
ftrace: add modify_ftrace_direct_multi_nolock
ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY
bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY

 arch/x86/net/bpf_jit_comp.c |  13 +-
 include/linux/bpf.h         |   8 ++
 include/linux/ftrace.h      |  79 +++++++++++
 kernel/bpf/trampoline.c     | 109 +++++++++++++--
 kernel/trace/ftrace.c       | 269 +++++++++++++++++++++++++++++++-----
 5 files changed, 424 insertions(+), 54 deletions(-)

--
2.30.2

I just realized that none of the live kernel patching folks are Cc'd on this thread. I think they will care much more about this than I do.

-- Steve

Quoted 36 lines · On Thu, 2 Jun 2022 12:37:01 -0700 Song Liu <[email protected]> wrote:

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 non-DIRECT IPMODIFY ftrace_ops and one non-IPMODIFY DIRECT ftrace_ops on the same kernel function at the same time. Please see 3/5 for more details on this.

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.

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

Song Liu (4):
ftrace: allow customized flags for ftrace_direct_multi ftrace_ops
ftrace: add modify_ftrace_direct_multi_nolock
ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY
bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY

arch/x86/net/bpf_jit_comp.c |  13 +-
include/linux/bpf.h         |   8 ++
include/linux/ftrace.h      |  79 +++++++++++
kernel/bpf/trampoline.c     | 109 +++++++++++++--
kernel/trace/ftrace.c       | 269 +++++++++++++++++++++++++++++++-----
5 files changed, 424 insertions(+), 54 deletions(-)

--
2.30.2

Quoted 4 lines · Steven Rostedt

I just realized that none of the live kernel patching folks are Cc'd on this thread. I think they will care much more about this than I do.

vger.kernel.org often drops my email when the CC list is too long. So I

try to keep the list short. In this case, since we are not changing live patch code, and there isn't any negative impact for live patch side, I didn't CC live patch folks.

I will at least CC live-patching@ in the next version.