[PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together
@ 2022-06-01 17:57 Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 1/5] ftrace: allow customized flags for ftrace_direct_multi ftrace_ops Song Liu
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Song Liu @ 2022-06-01 17:57 UTC (permalink / raw)
  To: netdev, bpf, linux-kernel
  Cc: ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

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     | 100 ++++++++++++--
 kernel/trace/ftrace.c       | 269 +++++++++++++++++++++++++++++++-----
 5 files changed, 416 insertions(+), 53 deletions(-)

--
2.30.2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 1/5] ftrace: allow customized flags for ftrace_direct_multi ftrace_ops
  2022-06-01 17:57 [PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together Song Liu
@ 2022-06-01 17:57 ` Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 2/5] ftrace: add modify_ftrace_direct_multi_nolock Song Liu
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Song Liu @ 2022-06-01 17:57 UTC (permalink / raw)
  To: netdev, bpf, linux-kernel
  Cc: ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

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 <song@kernel.org>
---
 kernel/trace/ftrace.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 2fcd17857ff6..afe782ae28d3 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 2/5] ftrace: add modify_ftrace_direct_multi_nolock
  2022-06-01 17:57 [PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 1/5] ftrace: allow customized flags for ftrace_direct_multi ftrace_ops Song Liu
@ 2022-06-01 17:57 ` Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Song Liu @ 2022-06-01 17:57 UTC (permalink / raw)
  To: netdev, bpf, linux-kernel
  Cc: ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

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 <song@kernel.org>
---
 include/linux/ftrace.h |  5 +++
 kernel/trace/ftrace.c  | 85 ++++++++++++++++++++++++++++++------------
 2 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 820500430eae..9023bf69f675 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -343,6 +343,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;
@@ -387,6 +388,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
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index afe782ae28d3..6a419f6bbbf0 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5601,22 +5601,8 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 }
 EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
 
-/**
- * modify_ftrace_direct_multi - Modify an existing direct 'multi' call
- * to call something else
- * @ops: The address of the struct ftrace_ops object
- * @addr: The address of the new trampoline to call at @ops functions
- *
- * This is used to unregister currently registered direct caller and
- * register new one @addr on functions registered in @ops object.
- *
- * Note there's window between ftrace_shutdown and ftrace_startup calls
- * where there will be no callbacks called.
- *
- * Returns: zero on success. Non zero on error, which includes:
- *  -EINVAL - The @ops object was not properly registered.
- */
-int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+static int
+__modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 {
 	struct ftrace_hash *hash;
 	struct ftrace_func_entry *entry, *iter;
@@ -5627,12 +5613,8 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 	int i, size;
 	int err;
 
-	if (check_direct_multi(ops))
+	if (WARN_ON_ONCE(!mutex_is_locked(&direct_mutex)))
 		return -EINVAL;
-	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
-		return -EINVAL;
-
-	mutex_lock(&direct_mutex);
 
 	/* Enable the tmp_ops to have the same functions as the direct ops */
 	ftrace_ops_init(&tmp_ops);
@@ -5640,7 +5622,7 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 
 	err = register_ftrace_function(&tmp_ops);
 	if (err)
-		goto out_direct;
+		return err;
 
 	/*
 	 * Now the ftrace_ops_list_func() is called to do the direct callers.
@@ -5664,7 +5646,64 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 	/* Removing the tmp_ops will add the updated direct callers to the functions */
 	unregister_ftrace_function(&tmp_ops);
 
- out_direct:
+	return err;
+}
+
+/**
+ * modify_ftrace_direct_multi_nolock - Modify an existing direct 'multi' call
+ * to call something else
+ * @ops: The address of the struct ftrace_ops object
+ * @addr: The address of the new trampoline to call at @ops functions
+ *
+ * This is used to unregister currently registered direct caller and
+ * register new one @addr on functions registered in @ops object.
+ *
+ * Note there's window between ftrace_shutdown and ftrace_startup calls
+ * where there will be no callbacks called.
+ *
+ * Caller should already have direct_mutex locked, so we don't lock
+ * direct_mutex here.
+ *
+ * Returns: zero on success. Non zero on error, which includes:
+ *  -EINVAL - The @ops object was not properly registered.
+ */
+int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr)
+{
+	if (check_direct_multi(ops))
+		return -EINVAL;
+	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
+		return -EINVAL;
+
+	return __modify_ftrace_direct_multi(ops, addr);
+}
+EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi_nolock);
+
+/**
+ * modify_ftrace_direct_multi - Modify an existing direct 'multi' call
+ * to call something else
+ * @ops: The address of the struct ftrace_ops object
+ * @addr: The address of the new trampoline to call at @ops functions
+ *
+ * This is used to unregister currently registered direct caller and
+ * register new one @addr on functions registered in @ops object.
+ *
+ * Note there's window between ftrace_shutdown and ftrace_startup calls
+ * where there will be no callbacks called.
+ *
+ * Returns: zero on success. Non zero on error, which includes:
+ *  -EINVAL - The @ops object was not properly registered.
+ */
+int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+{
+	int err;
+
+	if (check_direct_multi(ops))
+		return -EINVAL;
+	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
+		return -EINVAL;
+
+	mutex_lock(&direct_mutex);
+	err = __modify_ftrace_direct_multi(ops, addr);
 	mutex_unlock(&direct_mutex);
 	return err;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY
  2022-06-01 17:57 [PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 1/5] ftrace: allow customized flags for ftrace_direct_multi ftrace_ops Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 2/5] ftrace: add modify_ftrace_direct_multi_nolock Song Liu
@ 2022-06-01 17:57 ` Song Liu
  2022-06-01 22:01   ` kernel test robot
  2022-06-01 17:57 ` [PATCH bpf-next 4/5] bpf, x64: Allow to use caller address from stack Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
  4 siblings, 1 reply; 10+ messages in thread
From: Song Liu @ 2022-06-01 17:57 UTC (permalink / raw)
  To: netdev, bpf, linux-kernel
  Cc: ast, daniel, andrii, kernel-team, rostedt, jolsa, 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.

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 <song@kernel.org>
---
 include/linux/ftrace.h |  74 +++++++++++++++++
 kernel/trace/ftrace.c  | 179 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 242 insertions(+), 11 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 9023bf69f675..ec8e18e53515 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -189,6 +189,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 +212,78 @@ 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().
+ */
+
+/*
+ * 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,
+};
+
+/*
+ * 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
 };
 
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6a419f6bbbf0..868bbc753803 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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);
+
+	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))
+					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);
+
+			if (found_op) {
+				if (!op->ops_func) {
+					ret = -EBUSY;
+					goto err_out;
+				}
+				ret = op->ops_func(op, FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY);
+				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);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY
  2022-06-01 17:57 ` [PATCH bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
@ 2022-06-01 22:01   ` kernel test robot
  0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-06-01 22:01 UTC (permalink / raw)
  To: Song Liu, netdev, bpf, linux-kernel
  Cc: kbuild-all, ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

Hi Song,

I love your patch! Yet something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220602-020112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: parisc-randconfig-r005-20220531 (https://download.01.org/0day-ci/archive/20220602/202206020533.bBh0IXx6-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a710d92bb10a7a0376af57af15208ea1b4396545
        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/20220602-020112
        git checkout a710d92bb10a7a0376af57af15208ea1b4396545
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash kernel/trace/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   kernel/trace/ftrace.c: In function 'prepare_direct_functions_for_ipmodify':
   kernel/trace/ftrace.c:8005:21: error: 'direct_mutex' undeclared (first use in this function); did you mean 'event_mutex'?
    8005 |         mutex_lock(&direct_mutex);
         |                     ^~~~~~~~~~~~
         |                     event_mutex
   kernel/trace/ftrace.c:8005:21: note: each undeclared identifier is reported only once for each function it appears in
>> kernel/trace/ftrace.c:8007:19: error: 'struct ftrace_ops' has no member named 'func_hash'
    8007 |         hash = ops->func_hash->filter_hash;
         |                   ^~
>> kernel/trace/ftrace.c:8020:37: error: implicit declaration of function 'ops_references_ip' [-Werror=implicit-function-declaration]
    8020 |                                 if (ops_references_ip(op, ip)) {
         |                                     ^~~~~~~~~~~~~~~~~
>> kernel/trace/ftrace.c:8028:40: error: 'struct ftrace_ops' has no member named 'ops_func'
    8028 |                                 if (!op->ops_func) {
         |                                        ^~
   kernel/trace/ftrace.c:8032:41: error: 'struct ftrace_ops' has no member named 'ops_func'
    8032 |                                 ret = op->ops_func(op, FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY);
         |                                         ^~
   kernel/trace/ftrace.c: In function 'register_ftrace_function':
   kernel/trace/ftrace.c:8084:31: error: 'direct_mutex' undeclared (first use in this function); did you mean 'event_mutex'?
    8084 |                 mutex_unlock(&direct_mutex);
         |                               ^~~~~~~~~~~~
         |                               event_mutex
   cc1: some warnings being treated as errors


vim +8007 kernel/trace/ftrace.c

  7974	
  7975	/*
  7976	 * When registering ftrace_ops with IPMODIFY (not direct), it is necessary
  7977	 * to make sure it doesn't conflict with any direct ftrace_ops. If there is
  7978	 * existing direct ftrace_ops on a kernel function being patched, call
  7979	 * FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY on it to enable sharing.
  7980	 *
  7981	 * @ops:     ftrace_ops being registered.
  7982	 *
  7983	 * Returns:
  7984	 *         0 - @ops does have IPMODIFY or @ops itself is DIRECT, no change
  7985	 *             needed;
  7986	 *         1 - @ops has IPMODIFY, hold direct_mutex;
  7987	 *         -EBUSY - currently registered DIRECT ftrace_ops does not support
  7988	 *                  SHARE_IPMODIFY, we need to abort the register.
  7989	 *         -EAGAIN - cannot make changes to currently registered DIRECT
  7990	 *                   ftrace_ops at the moment, but we can retry later. This
  7991	 *                   is needed to avoid potential deadlocks.
  7992	 */
  7993	static int prepare_direct_functions_for_ipmodify(struct ftrace_ops *ops)
  7994		__acquires(&direct_mutex)
  7995	{
  7996		struct ftrace_func_entry *entry;
  7997		struct ftrace_hash *hash;
  7998		struct ftrace_ops *op;
  7999		int size, i, ret;
  8000	
  8001		if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY) ||
  8002		    (ops->flags & FTRACE_OPS_FL_DIRECT))
  8003			return 0;
  8004	
  8005		mutex_lock(&direct_mutex);
  8006	
> 8007		hash = ops->func_hash->filter_hash;
  8008		size = 1 << hash->size_bits;
  8009		for (i = 0; i < size; i++) {
  8010			hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
  8011				unsigned long ip = entry->ip;
  8012				bool found_op = false;
  8013	
  8014				mutex_lock(&ftrace_lock);
  8015				do_for_each_ftrace_op(op, ftrace_ops_list) {
  8016					if (!(op->flags & FTRACE_OPS_FL_DIRECT))
  8017						continue;
  8018					if (op->flags & FTRACE_OPS_FL_SHARE_IPMODIFY)
  8019						break;
> 8020					if (ops_references_ip(op, ip)) {
  8021						found_op = true;
  8022						break;
  8023					}
  8024				} while_for_each_ftrace_op(op);
  8025				mutex_unlock(&ftrace_lock);
  8026	
  8027				if (found_op) {
> 8028					if (!op->ops_func) {
  8029						ret = -EBUSY;
  8030						goto err_out;
  8031					}
  8032					ret = op->ops_func(op, FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY);
  8033					if (ret)
  8034						goto err_out;
  8035				}
  8036			}
  8037		}
  8038	
  8039		/*
  8040		 * Didn't find any overlap with any direct function, or the direct
  8041		 * function can share with ipmodify. Hold direct_mutex to make sure
  8042		 * this doesn't change until we are done.
  8043		 */
  8044		return 1;
  8045	
  8046	err_out:
  8047		mutex_unlock(&direct_mutex);
  8048		return ret;
  8049	

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 4/5] bpf, x64: Allow to use caller address from stack
  2022-06-01 17:57 [PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together Song Liu
                   ` (2 preceding siblings ...)
  2022-06-01 17:57 ` [PATCH bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
@ 2022-06-01 17:57 ` Song Liu
  2022-06-01 17:57 ` [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
  4 siblings, 0 replies; 10+ messages in thread
From: Song Liu @ 2022-06-01 17:57 UTC (permalink / raw)
  To: netdev, bpf, linux-kernel
  Cc: ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

From: Jiri Olsa <jolsa@kernel.org>

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 <jolsa@kernel.org>
Signed-off-by: Song Liu <song@kernel.org>
---
 arch/x86/net/bpf_jit_comp.c | 13 +++++++++----
 include/linux/bpf.h         |  5 +++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index f298b18a9a3d..c835a9f18fd8 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2130,10 +2130,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);
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 8e6092d0ea95..a6e06f384e81 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -733,6 +733,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 fexit programs only.
+ */
+#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.
  */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY
  2022-06-01 17:57 [PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together Song Liu
                   ` (3 preceding siblings ...)
  2022-06-01 17:57 ` [PATCH bpf-next 4/5] bpf, x64: Allow to use caller address from stack Song Liu
@ 2022-06-01 17:57 ` Song Liu
  2022-06-01 22:44   ` kernel test robot
                     ` (2 more replies)
  4 siblings, 3 replies; 10+ messages in thread
From: Song Liu @ 2022-06-01 17:57 UTC (permalink / raw)
  To: netdev, bpf, linux-kernel
  Cc: ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

This allows bpf trampoline to trace kernel functions with live patch.
Also, move bpf trampoline to *_ftrace_direct_multi APIs, which allows
setting different flags of ftrace_ops.

Signed-off-by: Song Liu <song@kernel.org>
---
 include/linux/bpf.h     |   3 ++
 kernel/bpf/trampoline.c | 100 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a6e06f384e81..20a8ed600ca6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -44,6 +44,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;
@@ -816,6 +817,7 @@ 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;
@@ -838,6 +840,7 @@ struct bpf_trampoline {
 	struct bpf_tramp_image *cur_image;
 	u64 selector;
 	struct module *mod;
+	bool indirect_call;
 };
 
 struct bpf_attach_target_info {
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 93c7675f0c9e..33d70d6ed165 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -27,6 +27,8 @@ static struct hlist_head trampoline_table[TRAMPOLINE_TABLE_SIZE];
 /* serializes access to trampoline_table */
 static DEFINE_MUTEX(trampoline_mutex);
 
+static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
+
 bool bpf_prog_has_trampoline(const struct bpf_prog *prog)
 {
 	enum bpf_attach_type eatype = prog->expected_attach_type;
@@ -87,8 +89,16 @@ static struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 	tr = kzalloc(sizeof(*tr), GFP_KERNEL);
 	if (!tr)
 		goto out;
+	tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
+	if (!tr->fops) {
+		kfree(tr);
+		tr = NULL;
+		goto out;
+	}
 
 	tr->key = key;
+	tr->fops->private = tr;
+	tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
 	INIT_HLIST_NODE(&tr->hlist);
 	hlist_add_head(&tr->hlist, head);
 	refcount_set(&tr->refcnt, 1);
@@ -126,7 +136,7 @@ static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
 	int ret;
 
 	if (tr->func.ftrace_managed)
-		ret = unregister_ftrace_direct((long)ip, (long)old_addr);
+		ret = unregister_ftrace_direct_multi(tr->fops, (long)old_addr);
 	else
 		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL);
 
@@ -135,15 +145,20 @@ static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
 	return ret;
 }
 
-static int modify_fentry(struct bpf_trampoline *tr, void *old_addr, void *new_addr)
+static int modify_fentry(struct bpf_trampoline *tr, void *old_addr, void *new_addr,
+			 bool lock_direct_mutex)
 {
 	void *ip = tr->func.addr;
 	int ret;
 
-	if (tr->func.ftrace_managed)
-		ret = modify_ftrace_direct((long)ip, (long)old_addr, (long)new_addr);
-	else
+	if (tr->func.ftrace_managed) {
+		if (lock_direct_mutex)
+			ret = modify_ftrace_direct_multi(tr->fops, (long)new_addr);
+		else
+			ret = modify_ftrace_direct_multi_nolock(tr->fops, (long)new_addr);
+	} else {
 		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, new_addr);
+	}
 	return ret;
 }
 
@@ -161,10 +176,15 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
 	if (bpf_trampoline_module_get(tr))
 		return -ENOENT;
 
-	if (tr->func.ftrace_managed)
-		ret = register_ftrace_direct((long)ip, (long)new_addr);
-	else
+	if (tr->func.ftrace_managed) {
+		ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 0);
+		ret = register_ftrace_direct_multi(tr->fops, (long)new_addr);
+		if (ret)
+			ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 1, 0);
+
+	} else {
 		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr);
+	}
 
 	if (ret)
 		bpf_trampoline_module_put(tr);
@@ -330,7 +350,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 +383,40 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	if (ip_arg)
 		flags |= BPF_TRAMP_F_IP_ARG;
 
+again:
+	if (tr->indirect_call)
+		flags |= BPF_TRAMP_F_ORIG_STACK;
+
 	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;
 
+	if (tr->indirect_call)
+		tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
+
 	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);
+
+	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;
+	}
+
 	if (err)
 		goto out;
 	if (tr->cur_image)
@@ -388,6 +428,41 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	return err;
 }
 
+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)
+	 *
+	 * 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))
+		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;
+}
+
+
 static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
 {
 	switch (prog->expected_attach_type) {
@@ -460,7 +535,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 +562,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 +610,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);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY
  2022-06-01 17:57 ` [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
@ 2022-06-01 22:44   ` kernel test robot
  2022-06-02  0:08   ` kernel test robot
  2022-06-02  1:30   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-06-01 22:44 UTC (permalink / raw)
  To: Song Liu, netdev, bpf, linux-kernel
  Cc: llvm, kbuild-all, ast, daniel, andrii, kernel-team, rostedt,
	jolsa, Song Liu

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/20220602-020112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-a015 (https://download.01.org/0day-ci/archive/20220602/202206020622.HnFjEObo-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c825abd6b0198fb088d9752f556a70705bc99dfd)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7edcf1c49617641579f2bc36b86c7d59bea20aef
        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/20220602-020112
        git checkout 7edcf1c49617641579f2bc36b86c7d59bea20aef
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash kernel/bpf/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/bpf/trampoline.c:30:66: warning: declaration of 'enum ftrace_ops_cmd' will not be visible outside of this function [-Wvisibility]
   static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
                                                                    ^
   kernel/bpf/trampoline.c:92:21: error: invalid application of 'sizeof' to an incomplete type 'struct ftrace_ops'
           tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
                              ^     ~~~~~~~~~~~~~~~~~~~
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:100:10: error: incomplete definition of type 'struct ftrace_ops'
           tr->fops->private = tr;
           ~~~~~~~~^
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:101:10: error: incomplete definition of type 'struct ftrace_ops'
           tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
           ~~~~~~~~^
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:397:11: error: incomplete definition of type 'struct ftrace_ops'
                   tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
                   ~~~~~~~~^
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:397:22: error: use of undeclared identifier 'FTRACE_OPS_FL_SHARE_IPMODIFY'
                   tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
                                      ^
   kernel/bpf/trampoline.c:415:11: error: incomplete definition of type 'struct ftrace_ops'
                   tr->fops->func = NULL;
                   ~~~~~~~~^
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:416:11: error: incomplete definition of type 'struct ftrace_ops'
                   tr->fops->trampoline = 0;
                   ~~~~~~~~^
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:431:67: warning: declaration of 'enum ftrace_ops_cmd' will not be visible outside of this function [-Wvisibility]
   static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
                                                                     ^
   kernel/bpf/trampoline.c:431:12: error: conflicting types for 'bpf_tramp_ftrace_ops_func'
   static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
              ^
   kernel/bpf/trampoline.c:30:12: note: previous declaration is here
   static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
              ^
   kernel/bpf/trampoline.c:431:82: error: variable has incomplete type 'enum ftrace_ops_cmd'
   static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
                                                                                    ^
   kernel/bpf/trampoline.c:431:67: note: forward declaration of 'enum ftrace_ops_cmd'
   static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
                                                                     ^
   kernel/bpf/trampoline.c:433:33: error: incomplete definition of type 'struct ftrace_ops'
           struct bpf_trampoline *tr = ops->private;
                                       ~~~^
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:448:7: error: use of undeclared identifier 'FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY'
           case FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY:
                ^
   kernel/bpf/trampoline.c:452:7: error: use of undeclared identifier 'FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY'
           case FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY:
                ^
   kernel/bpf/trampoline.c:454:11: error: incomplete definition of type 'struct ftrace_ops'
                   tr->fops->flags &= ~FTRACE_OPS_FL_SHARE_IPMODIFY;
                   ~~~~~~~~^
   include/linux/bpf.h:47:8: note: forward declaration of 'struct ftrace_ops'
   struct ftrace_ops;
          ^
   kernel/bpf/trampoline.c:454:23: error: use of undeclared identifier 'FTRACE_OPS_FL_SHARE_IPMODIFY'
                   tr->fops->flags &= ~FTRACE_OPS_FL_SHARE_IPMODIFY;
                                       ^
   2 warnings and 14 errors generated.


vim +30 kernel/bpf/trampoline.c

    29	
  > 30	static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
    31	

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY
  2022-06-01 17:57 ` [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
  2022-06-01 22:44   ` kernel test robot
@ 2022-06-02  0:08   ` kernel test robot
  2022-06-02  1:30   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-06-02  0:08 UTC (permalink / raw)
  To: Song Liu, netdev, bpf, linux-kernel
  Cc: kbuild-all, ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

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/20220602-020112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-a003 (https://download.01.org/0day-ci/archive/20220602/202206020707.jsHlBldB-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/7edcf1c49617641579f2bc36b86c7d59bea20aef
        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/20220602-020112
        git checkout 7edcf1c49617641579f2bc36b86c7d59bea20aef
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash kernel/bpf/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/bpf/trampoline.c:30:66: warning: 'enum ftrace_ops_cmd' declared inside parameter list will not be visible outside of this definition or declaration
      30 | static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
         |                                                                  ^~~~~~~~~~~~~~
   kernel/bpf/trampoline.c: In function 'bpf_trampoline_lookup':
   kernel/bpf/trampoline.c:92:35: error: invalid application of 'sizeof' to incomplete type 'struct ftrace_ops'
      92 |         tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
         |                                   ^~~~~~
   kernel/bpf/trampoline.c:100:17: error: invalid use of undefined type 'struct ftrace_ops'
     100 |         tr->fops->private = tr;
         |                 ^~
   kernel/bpf/trampoline.c:101:17: error: invalid use of undefined type 'struct ftrace_ops'
     101 |         tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
         |                 ^~
   kernel/bpf/trampoline.c: In function 'bpf_trampoline_update':
   kernel/bpf/trampoline.c:397:25: error: invalid use of undefined type 'struct ftrace_ops'
     397 |                 tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
         |                         ^~
   kernel/bpf/trampoline.c:397:36: error: 'FTRACE_OPS_FL_SHARE_IPMODIFY' undeclared (first use in this function)
     397 |                 tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/bpf/trampoline.c:397:36: note: each undeclared identifier is reported only once for each function it appears in
   kernel/bpf/trampoline.c:415:25: error: invalid use of undefined type 'struct ftrace_ops'
     415 |                 tr->fops->func = NULL;
         |                         ^~
   kernel/bpf/trampoline.c:416:25: error: invalid use of undefined type 'struct ftrace_ops'
     416 |                 tr->fops->trampoline = 0;
         |                         ^~
   kernel/bpf/trampoline.c: At top level:
   kernel/bpf/trampoline.c:431:67: warning: 'enum ftrace_ops_cmd' declared inside parameter list will not be visible outside of this definition or declaration
     431 | static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
         |                                                                   ^~~~~~~~~~~~~~
   kernel/bpf/trampoline.c:431:82: error: parameter 2 ('cmd') has incomplete type
     431 | static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
         |                                                              ~~~~~~~~~~~~~~~~~~~~^~~
   kernel/bpf/trampoline.c: In function 'bpf_tramp_ftrace_ops_func':
   kernel/bpf/trampoline.c:433:40: error: invalid use of undefined type 'struct ftrace_ops'
     433 |         struct bpf_trampoline *tr = ops->private;
         |                                        ^~
   kernel/bpf/trampoline.c:448:14: error: 'FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY' undeclared (first use in this function)
     448 |         case FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY:
         |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/bpf/trampoline.c:452:14: error: 'FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY' undeclared (first use in this function)
     452 |         case FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY:
         |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/bpf/trampoline.c:454:25: error: invalid use of undefined type 'struct ftrace_ops'
     454 |                 tr->fops->flags &= ~FTRACE_OPS_FL_SHARE_IPMODIFY;
         |                         ^~
   kernel/bpf/trampoline.c:454:37: error: 'FTRACE_OPS_FL_SHARE_IPMODIFY' undeclared (first use in this function)
     454 |                 tr->fops->flags &= ~FTRACE_OPS_FL_SHARE_IPMODIFY;
         |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +30 kernel/bpf/trampoline.c

    29	
  > 30	static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
    31	

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY
  2022-06-01 17:57 ` [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
  2022-06-01 22:44   ` kernel test robot
  2022-06-02  0:08   ` kernel test robot
@ 2022-06-02  1:30   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-06-02  1:30 UTC (permalink / raw)
  To: Song Liu, netdev, bpf, linux-kernel
  Cc: kbuild-all, ast, daniel, andrii, kernel-team, rostedt, jolsa, Song Liu

Hi Song,

I love your patch! Yet something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220602-020112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a006 (https://download.01.org/0day-ci/archive/20220602/202206020957.KETjl2xP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/7edcf1c49617641579f2bc36b86c7d59bea20aef
        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/20220602-020112
        git checkout 7edcf1c49617641579f2bc36b86c7d59bea20aef
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   kernel/bpf/trampoline.c: In function 'bpf_trampoline_lookup':
>> kernel/bpf/trampoline.c:101:17: error: 'struct ftrace_ops' has no member named 'ops_func'
     101 |         tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
         |                 ^~
   kernel/bpf/trampoline.c: In function 'bpf_trampoline_update':
>> kernel/bpf/trampoline.c:416:25: error: 'struct ftrace_ops' has no member named 'trampoline'
     416 |                 tr->fops->trampoline = 0;
         |                         ^~


vim +101 kernel/bpf/trampoline.c

    74	
    75	static struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
    76	{
    77		struct bpf_trampoline *tr;
    78		struct hlist_head *head;
    79		int i;
    80	
    81		mutex_lock(&trampoline_mutex);
    82		head = &trampoline_table[hash_64(key, TRAMPOLINE_HASH_BITS)];
    83		hlist_for_each_entry(tr, head, hlist) {
    84			if (tr->key == key) {
    85				refcount_inc(&tr->refcnt);
    86				goto out;
    87			}
    88		}
    89		tr = kzalloc(sizeof(*tr), GFP_KERNEL);
    90		if (!tr)
    91			goto out;
    92		tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
    93		if (!tr->fops) {
    94			kfree(tr);
    95			tr = NULL;
    96			goto out;
    97		}
    98	
    99		tr->key = key;
   100		tr->fops->private = tr;
 > 101		tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
   102		INIT_HLIST_NODE(&tr->hlist);
   103		hlist_add_head(&tr->hlist, head);
   104		refcount_set(&tr->refcnt, 1);
   105		mutex_init(&tr->mutex);
   106		for (i = 0; i < BPF_TRAMP_MAX; i++)
   107			INIT_HLIST_HEAD(&tr->progs_hlist[i]);
   108	out:
   109		mutex_unlock(&trampoline_mutex);
   110		return tr;
   111	}
   112	

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-02  1:31 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-01 17:57 [PATCH bpf-next 0/5] ftrace: host klp and bpf trampoline together Song Liu
2022-06-01 17:57 ` [PATCH bpf-next 1/5] ftrace: allow customized flags for ftrace_direct_multi ftrace_ops Song Liu
2022-06-01 17:57 ` [PATCH bpf-next 2/5] ftrace: add modify_ftrace_direct_multi_nolock Song Liu
2022-06-01 17:57 ` [PATCH bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
2022-06-01 22:01   ` kernel test robot
2022-06-01 17:57 ` [PATCH bpf-next 4/5] bpf, x64: Allow to use caller address from stack Song Liu
2022-06-01 17:57 ` [PATCH bpf-next 5/5] bpf: trampoline: support FTRACE_OPS_FL_SHARE_IPMODIFY Song Liu
2022-06-01 22:44   ` kernel test robot
2022-06-02  0:08   ` kernel test robot
2022-06-02  1:30   ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.