linux-kernel - Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the ftrace callback

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: <20201029145709.GD16774@alley>
Date:   Thu, 29 Oct 2020 15:57:09 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jiri Kosina <jikos@...nel.org>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        live-patching@...r.kernel.org
Subject: Re: [PATCH 6/9] livepatch/ftrace: Add recursion protection to the
 ftrace callback

On Thu 2020-10-29 14:51:06, Miroslav Benes wrote:
> On Wed, 28 Oct 2020, Steven Rostedt wrote:
> 
> > From: "Steven Rostedt (VMware)" <rostedt@...dmis.org>
> > 
> > If a ftrace callback does not supply its own recursion protection and
> > does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will
> > make a helper trampoline to do so before calling the callback instead of
> > just calling the callback directly.
> > 
> > The default for ftrace_ops is going to assume recursion protection unless
> > otherwise specified.

It might be my lack skills to read English. But the above sentence
sounds ambiguous to me. It is not clear to me who provides the
recursion protection by default. Could you please make it more
explicit, for example by:

"The default for ftrace_ops is going to change. It will expect that
handlers provide their own recursion protection."


> Hm, I've always thought that we did not need any kind of recursion 
> protection for our callback. It is marked as notrace and it does not call 
> anything traceable. In fact, it does not call anything. I even have a note 
> in my todo list to mark the callback as RECURSION_SAFE :)

Well, it calls WARN_ON_ONCE() ;-)

> At the same time, it probably does not hurt and the patch is still better 
> than what we have now without RECURSION_SAFE if I understand the patch set 
> correctly.

And better be on the safe side.


> > Cc: Josh Poimboeuf <jpoimboe@...hat.com>
> > Cc: Jiri Kosina <jikos@...nel.org>
> > Cc: Miroslav Benes <mbenes@...e.cz>
> > Cc: Petr Mladek <pmladek@...e.com>
> > Cc: Joe Lawrence <joe.lawrence@...hat.com>
> > Cc: live-patching@...r.kernel.org
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> > ---
> >  kernel/livepatch/patch.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> > index b552cf2d85f8..6c0164d24bbd 100644
> > --- a/kernel/livepatch/patch.c
> > +++ b/kernel/livepatch/patch.c
> > @@ -45,9 +45,13 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> >  	struct klp_ops *ops;
> >  	struct klp_func *func;
> >  	int patch_state;
> > +	int bit;
> >  
> >  	ops = container_of(fops, struct klp_ops, fops);
> >  
> > +	bit = ftrace_test_recursion_trylock();
> > +	if (bit < 0)
> > +		return;
> 
> This means that the original function will be called in case of recursion. 
> That's probably fair, but I'm wondering if we should at least WARN about 
> it.

Yeah, the early return might break the consistency model and
unexpected things might happen. We should be aware of it.
Please use:

	if (WARN_ON_ONCE(bit < 0))
		return;

WARN_ON_ONCE() might be part of the recursion. But it should happen
only once. IMHO, it is worth the risk.

Otherwise it looks good.

Best Regards,
Petr