On Tue, 25 Jan 2022 21:12:56 +0900
Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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;
> }
