linux-kernel - Re: [PATCH 0/4] x86: Fix ftrace recovery when code modification failed

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:	Fri, 21 Feb 2014 13:01:02 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Petr Mládek <pmladek@...e.cz>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH 0/4] x86: Fix ftrace recovery when code modification
 failed

On Fri, 21 Feb 2014 17:33:52 +0100
Petr Mládek <pmladek@...e.cz> wrote:
 
> Yes, it works but I would still do few more changes:
> 
>      + run_sync() also in ftrace_modify_code() in the recovery
>        part; I would solve this by moving the "out" label.

Sounds good.

> 
>      + print some warning in update_ftrace_func() when
>        ftrace_modify_code() fails; otherwise, the error
>        is silently ignored

The error is passed down to the callers. Any warning should happen
there. We can move that up to the generic code. Otherwise, this is a
x86 only fix, and we leave all the other archs broken.

ftrace_modify_all_code() looks to be where the warnings should be. And
yes, any warning should also call ftrace_bug(). That can be done via a
FTRACE_WARN_ON().

> 
>      + BUG when the breakpoint cannot be removed; the system
>        silently crash anyway because the breakpoint will not
>        be handled
> 
> I wonder if we should call FTRACE_WARN_ON_ONCE that calls
> "ftrace_kill" when update_ftrace_func() fails.

Don't need the _ONCE() version. Something like this (I'll let you make
the patch ;-)

	int ret = 0;

	if (update)
		ret = ftrace_update_ftrace_func(ftrace_ops_list_func);
	if (ret)
		goto out;

	if (command & FTRACE_UPDATE_CALLS)
		ret = ftrace_replace_code(1);
	else if (command & FTRACE_DISABLE_CALLS)
		ret = ftrace_replace_code(0);

	if (ret)
		goto out;

	if (update && ftrace_trace_function != ftrace_ops_list_func) {
		function_trace_op = set_function_trace_op;
		smp_wmb();
		/* If irqs are disabled, we are in stop machine */
		if (!irqs_disabled())
			smp_call_function(ftrace_sync_ipi, NULL, 1);
		ret = ftrace_update_ftrace_func(ftrace_trace_function);
	}

	if (ret)
		goto out;

	if (command & FTRACE_START_FUNC_RET)
		ret = ftrace_enable_ftrace_graph_caller();
	else if (command & FTRACE_STOP_FUNC_RET)
		ret = ftrace_disable_ftrace_graph_caller();

out:
	FTRACE_WARN_ON(ret);

Or if you want to keep where it failed, just replace the if (ret) goto
out, with:

	if (FTRACE_WARN_ON(ret))
		return;

That may actually be better.

I've already added my patch. I'll push it up to my ftrace/urgent branch
at

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git

And you can write a patch based on that.

Thanks!

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/