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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Jul 2022 22:28:30 +0000
From:   Song Liu <songliubraving@...com>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     Petr Mladek <pmladek@...e.com>, kernel test robot <lkp@...el.com>,
        Song Liu <song@...nel.org>, bpf <bpf@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
        "kbuild-all@...ts.01.org" <kbuild-all@...ts.01.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>,
        "jolsa@...nel.org" <jolsa@...nel.org>
Subject: Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on
 the same function



> On Jul 19, 2022, at 11:32 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> On Mon, 18 Jul 2022 16:59:51 +0000
> Song Liu <songliubraving@...com> wrote:
> 
>>>> 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?
> 
> Agreed. I'm not sure why I didn't notice this in the other versions.
> Probably was looking too much at the other logic. :-/
> 
>>> 
>>> 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. 
> 
> No need. Just take the direct_mutex, and perhaps add a:
> 
> 	lockdep_assert_held(&direct_mutex);
> 
> in the prepare_direct_functions_for_ipmodify().
> 
> This is far from a fast path to do any tricks in trying to optimize it.

Got it. I will fix these and send v5. 

Thanks,
Song