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

[Song Liu <songliubraving@fb.com>]

> On Jul 18, 2022, at 6:19 AM, Petr Mladek <pmladek@suse.com> wrote:
> 
> 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@intel.com/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@intel.com>
>> 
>> 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;

Yeah, we can actually do something like this. We can also move the
ops->flags & FTRACE_OPS_FL_IPMODIFY check to 
register_ftrace_function(), so we only lock direct_mutex when when
it is necessary. 

> 
> 
>>  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?

Unfortunately, we cannot do this. ftrace code cannot look up 
bpf trampolines without locking direct_mutex. 

Thanks,
Song