Message-Id: <20220130160932.30f7d4bd0af58eda4ea263c8@kernel.org>
Date:   Sun, 30 Jan 2022 16:09:32 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Jiri Olsa <jolsa@...hat.com>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Naveen N . Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v6 07/10] fprobe: Add exit_handler support

On Fri, 28 Jan 2022 23:33:15 +0900
Masami Hiramatsu <mhiramat@...nel.org> wrote:

> Add exit_handler to fprobe. fprobe + rethook allows us to hook the kernel
> function return. The rethook will be enabled only if the
> fprobe::exit_handler is set.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> ---
>  Changes in v6:
>   - Update according to the fprobe update.
>  Changes in v5:
>   - Add dependency for HAVE_RETHOOK.
>  Changes in v4:
>   - Check fprobe is disabled in the exit handler.
>  Changes in v3:
>   - Make sure to clear rethook->data before free.
>   - Handler checks the data is not NULL.
>   - Free rethook only if the rethook is using.
> ---
>  include/linux/fprobe.h |    6 ++
>  kernel/trace/Kconfig   |    2 +
>  kernel/trace/fprobe.c  |  127 +++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 123 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> index b920dc1b2969..acfdcc37acf6 100644
> --- a/include/linux/fprobe.h
> +++ b/include/linux/fprobe.h
> @@ -5,19 +5,25 @@
>  
>  #include <linux/compiler.h>
>  #include <linux/ftrace.h>
> +#include <linux/rethook.h>
>  
>  /**
>   * struct fprobe - ftrace based probe.
>   * @ops: The ftrace_ops.
>   * @nmissed: The counter for missing events.
>   * @flags: The status flag.
> + * @rethook: The rethook data structure. (internal data)
>   * @entry_handler: The callback function for function entry.
> + * @exit_handler: The callback function for function exit.
>   */
>  struct fprobe {
>  	struct ftrace_ops	ops;
>  	unsigned long		nmissed;
>  	unsigned int		flags;
> +	struct rethook		*rethook;
> +
>  	void (*entry_handler)(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs);
> +	void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs);
>  };
>  
>  #define FPROBE_FL_DISABLED	1
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 9e66fd29d94e..3c1f808969f1 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -245,6 +245,8 @@ config FPROBE
>  	bool "Kernel Function Probe (fprobe)"
>  	depends on FUNCTION_TRACER
>  	depends on DYNAMIC_FTRACE_WITH_REGS
> +	depends on HAVE_RETHOOK
> +	select RETHOOK
>  	default n
>  	help
>  	  This option enables kernel function probe (fprobe) based on ftrace,
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 081aef6bf531..408dcb6503fe 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -8,12 +8,22 @@
>  #include <linux/fprobe.h>
>  #include <linux/kallsyms.h>
>  #include <linux/kprobes.h>
> +#include <linux/rethook.h>
>  #include <linux/slab.h>
>  #include <linux/sort.h>
>  
> +#include "trace.h"
> +
> +struct fprobe_rethook_node {
> +	struct rethook_node node;
> +	unsigned long entry_ip;
> +};
> +
>  static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
>  			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
>  {
> +	struct fprobe_rethook_node *fpr;
> +	struct rethook_node *rh;
>  	struct fprobe *fp;
>  	int bit;
>  
> @@ -30,10 +40,37 @@ static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
>  	if (fp->entry_handler)
>  		fp->entry_handler(fp, ip, ftrace_get_regs(fregs));
>  
> +	if (fp->exit_handler) {
> +		rh = rethook_try_get(fp->rethook);
> +		if (!rh) {
> +			fp->nmissed++;
> +			goto out;
> +		}
> +		fpr = container_of(rh, struct fprobe_rethook_node, node);
> +		fpr->entry_ip = ip;
> +		rethook_hook(rh, ftrace_get_regs(fregs));
> +	}
> +
> +out:
>  	ftrace_test_recursion_unlock(bit);
>  }
>  NOKPROBE_SYMBOL(fprobe_handler);
>  
> +static void fprobe_exit_handler(struct rethook_node *rh, void *data,
> +				struct pt_regs *regs)
> +{
> +	struct fprobe *fp = (struct fprobe *)data;
> +	struct fprobe_rethook_node *fpr;
> +
> +	if (!fp || fprobe_disabled(fp))
> +		return;
> +
> +	fpr = container_of(rh, struct fprobe_rethook_node, node);
> +
> +	fp->exit_handler(fp, fpr->entry_ip, regs);
> +}
> +NOKPROBE_SYMBOL(fprobe_exit_handler);
> +
>  /* Convert ftrace location address from symbols */
>  static unsigned long *get_ftrace_locations(const char **syms, int num)
>  {
> @@ -76,6 +113,48 @@ static void fprobe_init(struct fprobe *fp)
>  	fp->ops.flags |= FTRACE_OPS_FL_SAVE_REGS;
>  }
>  
> +static int fprobe_init_rethook(struct fprobe *fp, int num)
> +{
> +	int i, size;
> +
> +	if (num < 0)
> +		return -EINVAL;
> +
> +	if (!fp->exit_handler) {
> +		fp->rethook = NULL;
> +		return 0;
> +	}
> +
> +	/* Initialize rethook if needed */
> +	size = num * num_possible_cpus() * 2;
> +	if (size < 0)
> +		return -E2BIG;
> +
> +	fp->rethook = rethook_alloc((void *)fp, fprobe_exit_handler);
> +	for (i = 0; i < size; i++) {
> +		struct rethook_node *node;
> +
> +		node = kzalloc(sizeof(struct fprobe_rethook_node), GFP_KERNEL);
> +		if (!node) {
> +			rethook_free(fp->rethook);
> +			fp->rethook = NULL;
> +			return -ENOMEM;
> +		}
> +		rethook_add_node(fp->rethook, node);
> +	}
> +	return 0;
> +}
> +
> +static void fprobe_fail_cleanup(struct fprobe *fp)
> +{
> +	if (fp->rethook) {
> +		/* Don't need to cleanup rethook->handler because this is not used. */
> +		rethook_free(fp->rethook);
> +		fp->rethook = NULL;
> +	}
> +	ftrace_free_filter(&fp->ops);
> +}
> +
>  /**
>   * register_fprobe() - Register fprobe to ftrace by pattern.
>   * @fp: A fprobe data structure to be registered.
> @@ -89,6 +168,7 @@ static void fprobe_init(struct fprobe *fp)
>   */
>  int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter)
>  {
> +	struct ftrace_hash *hash;
>  	unsigned char *str;
>  	int ret, len;
>  
> @@ -113,10 +193,21 @@ int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter
>  			goto out;
>  	}
>  
> -	ret = register_ftrace_function(&fp->ops);
> +	/* TODO:
> +	 * correctly calculate the total number of filtered symbols
> +	 * from both filter and notfilter.
> +	 */
> +	hash = fp->ops.local_hash.filter_hash;
> +	if (WARN_ON_ONCE(!hash))
> +		goto out;
> +
> +	ret = fprobe_init_rethook(fp, (int)hash->count);
> +	if (!ret)
> +		ret = register_ftrace_function(&fp->ops);
> +
>  out:
>  	if (ret)
> -		ftrace_free_filter(&fp->ops);
> +		fprobe_fail_cleanup(fp);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(register_fprobe);
> @@ -144,12 +235,15 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num)
>  	fprobe_init(fp);
>  
>  	ret = ftrace_set_filter_ips(&fp->ops, addrs, num, 0, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = fprobe_init_rethook(fp, num);
>  	if (!ret)
>  		ret = register_ftrace_function(&fp->ops);
>  
>  	if (ret)
> -		ftrace_free_filter(&fp->ops);
> -
> +		fprobe_fail_cleanup(fp);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(register_fprobe_ips);
> @@ -179,14 +273,16 @@ int register_fprobe_syms(struct fprobe *fp, const char **syms, int num)
>  		return PTR_ERR(addrs);
>  
>  	ret = ftrace_set_filter_ips(&fp->ops, addrs, num, 0, 0);
> +	kfree(addrs);
>  	if (ret)
> -		goto out;
> -	ret = register_ftrace_function(&fp->ops);
> -	if (ret)
> -		ftrace_free_filter(&fp->ops);
> +		return ret;
>  
> -out:
> -	kfree(addrs);
> +	ret = fprobe_init_rethook(fp, num);
> +	if (!ret)
> +		ret = register_ftrace_function(&fp->ops);
> +
> +	if (ret)
> +		fprobe_fail_cleanup(fp);
>  
>  	return ret;
>  }
> @@ -210,9 +306,16 @@ int unregister_fprobe(struct fprobe *fp)
>  		return -EINVAL;
>  
>  	ret = unregister_ftrace_function(&fp->ops);
> +	if (ret < 0)
> +		return ret;
>  
> -	if (!ret)
> -		ftrace_free_filter(&fp->ops);
> +	if (fp->rethook) {
> +		/* Make sure to clear rethook->data before freeing. */
> +		WRITE_ONCE(fp->rethook->data, NULL);
> +		barrier();
> +		rethook_free(fp->rethook);
> +	}

Oops, I felt uncomfortable on this part, and I found that this rethook_free()
must be called before unregister_ftrace_function() so that no more rethook will
be used, and also need to wait for rcu before returning (this is also done
in unregister_ftrace_function().)

Let me update this part.

Thank you,


> +	ftrace_free_filter(&fp->ops);
>  
>  	return ret;
>  }
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists