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

[Steven Rostedt <rostedt@goodmis.org>]

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;
>  	}