Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

[Petr Mladek]

On Mon 2022-07-18 15:42:25, kernel test robot wrote:
> Hi Song,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on bpf-next/master]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220718/202207181552.VuKfz9zg-lkp@xxxxxxxxx/config)
> compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/intel-lab-lkp/linux/commit/9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
>         git checkout 9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> 
> All warnings (new ones prefixed by >>):
> 
>    kernel/trace/ftrace.c: In function 'register_ftrace_function':
> >> kernel/trace/ftrace.c:8197:14: warning: variable 'direct_mutex_locked' set but not used [-Wunused-but-set-variable]
>     8197 |         bool direct_mutex_locked = false;
>          |              ^~~~~~~~~~~~~~~~~~~
> 
> 
> vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
> 
>   8182	
>   8183	/**
>   8184	 * register_ftrace_function - register a function for profiling
>   8185	 * @ops:	ops structure that holds the function for profiling.
>   8186	 *
>   8187	 * Register a function to be called by all functions in the
>   8188	 * kernel.
>   8189	 *
>   8190	 * Note: @ops->func and all the functions it calls must be labeled
>   8191	 *       with "notrace", otherwise it will go into a
>   8192	 *       recursive loop.
>   8193	 */
>   8194	int register_ftrace_function(struct ftrace_ops *ops)
>   8195		__releases(&direct_mutex)
>   8196	{
> > 8197		bool direct_mutex_locked = false;
>   8198		int ret;
>   8199	
>   8200		ftrace_ops_init(ops);
>   8201	
>   8202		ret = prepare_direct_functions_for_ipmodify(ops);
>   8203		if (ret < 0)
>   8204			return ret;
>   8205		else if (ret == 1)
>   8206			direct_mutex_locked = true;

Honestly, this is another horrible trick. Would it be possible to
call prepare_direct_functions_for_ipmodify() with direct_mutex
already taken?

I mean something like:

	mutex_lock(&direct_mutex);

	ret = prepare_direct_functions_for_ipmodify(ops);
	if (ret)
		goto out:

	mutex_lock(&ftrace_lock);
	ret = ftrace_startup(ops, 0);
	mutex_unlock(&ftrace_lock);

out:
	mutex_unlock(&direct_mutex);
	return ret;


>   8208		mutex_lock(&ftrace_lock);
>   8209	
>   8210		ret = ftrace_startup(ops, 0);
>   8211	
>   8212		mutex_unlock(&ftrace_lock);
>   8213	

Would be possible to handle tr->mutex the same way to avoid
the trylock? I mean to take it in advance before direct_mutex?

Best Regards,
Petr