From 03e9ef9281df3c53dd2501e6200316494c4ed7a2 Mon Sep 17 00:00:00 2001 From: Song Liu Date: Fri, 1 Apr 2022 01:11:01 +0000 Subject: [PATCH] ftrace_direct (used by bpf trampoline) conflicts with live patch Hi Steven, We hit an issue with bpf trampoline and kernel live patch on the same function. Basically, we have tracing and live patch on the same function. If we use kprobe (over ftrace) for tracing, it works fine with live patch. However, fentry on the same function does not work with live patch (the one comes later fails to attach). After digging into this, I found this is because bpf trampoline uses register_ftrace_direct, which enables IPMODIFY by default. OTOH, it seems that BPF doesn't really need IPMODIFY. As BPF trampoline does a "goto do_fexit" in jit for BPF_TRAMP_MODIFY_RETURN. IIUC, we can let bpf trampoline and live patch work together with an ipmodify-less version of register_ftrace_direct, like attached below. Does this make sense to you? Did I miss something? Thanks in advance, Song --- include/linux/ftrace.h | 2 ++ kernel/bpf/trampoline.c | 4 +-- kernel/trace/ftrace.c | 74 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index ed8cf433a46a3..46c40f0e0368a 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -326,6 +326,8 @@ struct dyn_ftrace; extern int ftrace_direct_func_count; int register_ftrace_direct(unsigned long ip, unsigned long addr); int unregister_ftrace_direct(unsigned long ip, unsigned long addr); +int register_ftrace_direct_no_ipmodify(unsigned long ip, unsigned long addr); +int unregister_ftrace_direct_no_ipmodify(unsigned long ip, unsigned long addr); int modify_ftrace_direct(unsigned long ip, unsigned long old_addr, unsigned long new_addr); struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr); int ftrace_modify_direct_caller(struct ftrace_func_entry *entry, diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index ada97751ae1b2..52ff503692cb6 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -123,7 +123,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_no_ipmodify((long)ip, (long)old_addr); else ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL); @@ -159,7 +159,7 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr) return -ENOENT; if (tr->func.ftrace_managed) - ret = register_ftrace_direct((long)ip, (long)new_addr); + ret = register_ftrace_direct_no_ipmodify((long)ip, (long)new_addr); else ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr); diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 4f1d2f5e72634..2d61e50f53177 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -2467,6 +2467,20 @@ struct ftrace_ops direct_ops = { */ .trampoline = FTRACE_REGS_ADDR, }; + +struct ftrace_ops no_ipmodify_direct_ops = { + .func = call_direct_funcs, + .flags = FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS + | FTRACE_OPS_FL_PERMANENT, + /* + * By declaring the main trampoline as this trampoline + * it will never have one allocated for it. Allocated + * trampolines should not call direct functions. + * The direct_ops should only be called by the builtin + * ftrace_regs_caller trampoline. + */ + .trampoline = FTRACE_REGS_ADDR, +}; #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */ /** @@ -5126,6 +5140,9 @@ static struct ftrace_direct_func *ftrace_alloc_direct_func(unsigned long addr) return direct; } +static int __register_ftrace_direct(unsigned long ip, unsigned long addr, + struct ftrace_ops *ops); + /** * register_ftrace_direct - Call a custom trampoline directly * @ip: The address of the nop at the beginning of a function @@ -5144,6 +5161,12 @@ static struct ftrace_direct_func *ftrace_alloc_direct_func(unsigned long addr) * -ENOMEM - There was an allocation failure. */ int register_ftrace_direct(unsigned long ip, unsigned long addr) +{ + return __register_ftrace_direct(ip, addr, &direct_ops); +} + +static int __register_ftrace_direct(unsigned long ip, unsigned long addr, + struct ftrace_ops *ops) { struct ftrace_direct_func *direct; struct ftrace_func_entry *entry; @@ -5194,14 +5217,14 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr) if (!entry) goto out_unlock; - ret = ftrace_set_filter_ip(&direct_ops, ip, 0, 0); + ret = ftrace_set_filter_ip(ops, ip, 0, 0); if (ret) remove_hash_entry(direct_functions, entry); - if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) { - ret = register_ftrace_function(&direct_ops); + if (!ret && !(ops->flags & FTRACE_OPS_FL_ENABLED)) { + ret = register_ftrace_function(ops); if (ret) - ftrace_set_filter_ip(&direct_ops, ip, 1, 0); + ftrace_set_filter_ip(ops, ip, 1, 0); } if (ret) { @@ -5230,6 +5253,29 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr) } EXPORT_SYMBOL_GPL(register_ftrace_direct); +/** + * register_ftrace_direct_no_ipmodify - Call a custom trampoline directly. + * The custom trampoline should not use IP_MODIFY. + * @ip: The address of the nop at the beginning of a function + * @addr: The address of the trampoline to call at @ip + * + * This is used to connect a direct call from the nop location (@ip) + * at the start of ftrace traced functions. The location that it calls + * (@addr) must be able to handle a direct call, and save the parameters + * of the function being traced, and restore them (or inject new ones + * if needed), before returning. + * + * Returns: + * 0 on success + * -EBUSY - Another direct function is already attached (there can be only one) + * -ENODEV - @ip does not point to a ftrace nop location (or not supported) + * -ENOMEM - There was an allocation failure. + */ +int register_ftrace_direct_no_ipmodify(unsigned long ip, unsigned long addr) +{ + return __register_ftrace_direct(ip, addr, &no_ipmodify_direct_ops); +} + static struct ftrace_func_entry *find_direct_entry(unsigned long *ip, struct dyn_ftrace **recp) { @@ -5257,7 +5303,21 @@ static struct ftrace_func_entry *find_direct_entry(unsigned long *ip, return entry; } +static int __unregister_ftrace_direct(unsigned long ip, unsigned long addr, + struct ftrace_ops *ops); + int unregister_ftrace_direct(unsigned long ip, unsigned long addr) +{ + return __unregister_ftrace_direct(ip, addr, &direct_ops); +} + +int unregister_ftrace_direct_no_ipmodify(unsigned long ip, unsigned long addr) +{ + return __unregister_ftrace_direct(ip, addr, &no_ipmodify_direct_ops); +} + +static int __unregister_ftrace_direct(unsigned long ip, unsigned long addr, + struct ftrace_ops *ops) { struct ftrace_direct_func *direct; struct ftrace_func_entry *entry; @@ -5274,11 +5334,11 @@ int unregister_ftrace_direct(unsigned long ip, unsigned long addr) if (!entry) goto out_unlock; - hash = direct_ops.func_hash->filter_hash; + hash = ops->func_hash->filter_hash; if (hash->count == 1) - unregister_ftrace_function(&direct_ops); + unregister_ftrace_function(ops); - ret = ftrace_set_filter_ip(&direct_ops, ip, 1, 0); + ret = ftrace_set_filter_ip(ops, ip, 1, 0); WARN_ON(ret);