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]

Message-ID: <20220719143210.08f9922b@gandalf.local.home>
Date:   Tue, 19 Jul 2022 14:32:10 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Song Liu <songliubraving@...com>
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 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.

-- Steve