linux-kernel - Re: [RFC][PATCH 1/2] ftrace: Add modify_ftrace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191115215125.mbqv7taqnx376yed@ast-mbp.dhcp.thefacebook.com>
Date:   Fri, 15 Nov 2019 13:51:26 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>
Subject: Re: [RFC][PATCH 1/2] ftrace: Add modify_ftrace_direct()

On Thu, Nov 14, 2019 at 02:46:37PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@...dmis.org>
> 
> Add a new function modify_ftrace_direct() that will allow a user to update
> an existing direct caller to a new trampoline, without missing hits due to
> unregistering one and then adding another.
> 
> Link: https://lore.kernel.org/r/20191109022907.6zzo6orhxpt5n2sv@ast-mbp.dhcp.thefacebook.com
> 
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@...il.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> ---
>  include/linux/ftrace.h |  6 ++++
>  kernel/trace/ftrace.c  | 78 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 55647e185141..73eb2e93593f 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -250,6 +250,7 @@ static inline void ftrace_free_mem(struct module *mod, void *start, void *end) {
>  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 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);
>  #else
>  # define ftrace_direct_func_count 0
> @@ -261,6 +262,11 @@ static inline int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
>  {
>  	return -ENODEV;
>  }
> +static inline int modify_ftrace_direct(unsigned long ip,
> +				       unsigned long old_addr, unsigned long new_addr)
> +{
> +	return -ENODEV;
> +}
>  static inline struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr)
>  {
>  	return NULL;
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 82ef8d60a42b..834f3556ea1e 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5160,6 +5160,84 @@ int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(unregister_ftrace_direct);
> +
> +static struct ftrace_ops stub_ops = {
> +	.func		= ftrace_stub,
> +};
> +
> +/**
> + * modify_ftrace_direct - Modify an existing direct call to call something else
> + * @ip: The instruction pointer to modify
> + * @old_addr: The address that the current @ip calls directly
> + * @new_addr: The address that the @ip should call
> + *
> + * This modifies a ftrace direct caller at an instruction pointer without
> + * having to disable it first. The direct call will switch over to the
> + * @new_addr without missing anything.
> + *
> + * Returns: zero on success. Non zero on error, which includes:
> + *  -ENODEV : the @ip given has no direct caller attached
> + *  -EINVAL : the @old_addr does not match the current direct caller
> + */
> +int modify_ftrace_direct(unsigned long ip,
> +			 unsigned long old_addr, unsigned long new_addr)
> +{
> +	struct ftrace_func_entry *entry;
> +	struct dyn_ftrace *rec;
> +	int ret = -ENODEV;
> +
> +	mutex_lock(&direct_mutex);
> +	entry = __ftrace_lookup_ip(direct_functions, ip);
> +	if (!entry) {
> +		/* OK if it is off by a little */
> +		rec = lookup_rec(ip, ip);
> +		if (!rec || rec->ip == ip)
> +			goto out_unlock;
> +
> +		entry = __ftrace_lookup_ip(direct_functions, rec->ip);
> +		if (!entry)
> +			goto out_unlock;
> +
> +		ip = rec->ip;
> +		WARN_ON(!(rec->flags & FTRACE_FL_DIRECT));
> +	}
> +
> +	ret = -EINVAL;
> +	if (entry->direct != old_addr)
> +		goto out_unlock;
> +
> +	/*
> +	 * By setting a stub function at the same address, we force
> +	 * the code to call the iterator and the direct_ops helper.
> +	 * This means that @ip does not call the direct call, and
> +	 * we can simply modify it.
> +	 */
> +	ret = ftrace_set_filter_ip(&stub_ops, ip, 0, 0);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = register_ftrace_function(&stub_ops);
> +	if (ret) {
> +		ftrace_set_filter_ip(&stub_ops, ip, 1, 0);
> +		goto out_unlock;
> +	}
> +
> +	entry->direct = new_addr;
> +
> +	/*
> +	 * By removing the stub, we put back the direct call, calling
> +	 * the @new_addr.
> +	 */
> +	unregister_ftrace_function(&stub_ops);
> +	ftrace_set_filter_ip(&stub_ops, ip, 1, 0);

Thanks a lot for implementing it.
Switching to iterator just to modify the call.. hmm.
So "call direct_bpf_A" gets replaced to "call ftrace_stub" to do the iterator
only to patch "call direct_bpf_B" later. I'm struggling to see why do that when
arch can provide call to call rewrite easily. x86 and others have such ability
already. I don't understand why you want to sacrifice simplicity here.
Anyway with all 3 apis (register, modify, unreg) it looks complete.
I'll start playing with it on Monday.