Re: [PATCH v5 7/9] fprobe: Add exit_handler support

Steven Rostedt
2022-01-25T17:09:58Z

On Tue, 25 Jan 2022 21:12:56 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Add exit_handler to fprobe. fprobe + rethook allows us
> to hook the kernel function return without fgraph tracer.
> Eventually, the fgraph tracer will be generic array based
> return hooking and fprobe may use it if user requests.
> Since both array-based approach and list-based approach
> have Pros and Cons, (e.g. memory consumption v.s. less
> missing events) it is better to keep both but fprobe
> will provide the same exit-handler interface.

Again the 55 character width ;-)

>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
> 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.
> ---

> @@ -82,6 +117,7 @@ static int convert_func_addresses(struct fprobe *fp)
> */
> int register_fprobe(struct fprobe *fp)
> {
> + unsigned int i, size;
> int ret;
>
> if (!fp || !fp->nentry || (!fp->syms && !fp->addrs) ||
> @@ -96,10 +132,29 @@ int register_fprobe(struct fprobe *fp)
> fp->ops.func = fprobe_handler;
> fp->ops.flags = FTRACE_OPS_FL_SAVE_REGS;
>
> + /* Initialize rethook if needed */
> + if (fp->exit_handler) {
> + size = fp->nentry * num_possible_cpus() * 2;
> + fp->rethook = rethook_alloc((void *)fp, fprobe_exit_handler);

Shouldn't we check if fp->rethook succeeded to be allocated?

> + 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);
> + ret = -ENOMEM;
> + goto out;
> + }
> + rethook_add_node(fp->rethook, node);
> + }
> + } else
> + fp->rethook = NULL;
> +
> ret = ftrace_set_filter_ips(&fp->ops, fp->addrs, fp->nentry, 0, 0);
> if (!ret)
> ret = register_ftrace_function(&fp->ops);
>
> +out:
> if (ret < 0 && fp->syms) {
> kfree(fp->addrs);
> fp->addrs = NULL;
> @@ -125,8 +180,16 @@ int unregister_fprobe(struct fprobe *fp)
> return -EINVAL;
>
> ret = unregister_ftrace_function(&fp->ops);
> + if (ret < 0)
> + return ret;

If we fail to unregister the fp->ops, we do not free the allocated nodes
above?

-- Steve

>
> - if (!ret && fp->syms) {
> + if (fp->rethook) {
> + /* Make sure to clear rethook->data before freeing. */
> + WRITE_ONCE(fp->rethook->data, NULL);
> + barrier();
> + rethook_free(fp->rethook);
> + }
> + if (fp->syms) {
> kfree(fp->addrs);
> fp->addrs = NULL;
> }