Message-ID: <20090223173108.GB1441@Krystal>
Date:	Mon, 23 Feb 2009 12:31:08 -0500
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Andi Kleen <andi@...stfloor.org>, linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Arjan van de Ven <arjan@...radead.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"H. Peter Anvin" <hpa@...or.com>,
	Steven Rostedt <srostedt@...hat.com>
Subject: Re: [PATCH 4/6] ftrace, x86: make kernel text writable only for
	conversions

* Steven Rostedt (rostedt@...dmis.org) wrote:
> 
> On Mon, 23 Feb 2009, Mathieu Desnoyers wrote:
> > It can, by using your function tracer. It has a mode where it can
> > enable/disable a filter in a callback connected on tracepoints. This
> > filter is then used to enable detailed function tracing for a short time
> > window. Also, you could think of tracing every function calls with
> > LTTng's flight recorder mode, which only spins in memory overwriting the
> > oldest information. That would provide snapshots on demand of the last
> > functions called.
> > 
> > > Now, yes, if you only select a few functions, there's no noticeable 
> > > overhead. And yes then you would need to do the stop_machine anyway, and 
> > > there will be a small window where the kernel text will be writable. 
> > > Tracing only a small set of functions (say a few 100) is not much of an 
> > > overhead, and I could see that being done on a production system.
> > > 
> > 
> > This is what LTTng can do today. But that involves the function tracer
> > stop_machine() call, which I dislike.
> 
> What's wrong with stop_machine?  Specifically, what do you dislike about 
> it?
> 
> > 
> > > > 
> > > > I agree that the racy time window is not that large and is not really a
> > > > security concern, but it's still just annoying.
> > > 
> > > Annoying? how so?
> > > 
> > > Again, the stop_machine part has nothing to do with DEBUG_RODATA, it is 
> > > about the safest and easiest way to modify kernel text.
> > > 
> > 
> > We are running in circles here because there is no real argument
> > brought.
> > 
> > 1 - You claim that changing the kernel's mapping, which has been
> > pointed out as an intrusive kernel modification, is faster than using a
> > text-poke-like approach. Please provide numbers to support such claims.
> 
> Hmm, lets see. I simply set a bit in the PTE mappings. There's not many, 
> since a lot are 2M pages, for x86_64. Call stop_machine, and now I can 
> modify 1 or 20,000 locations. Set the PTE bit back. Note, the changing of 
> the bits are only done when CONFIG_DEBUG_RODATA is set.
> 
> text_poke requires allocating a page. Map the page into memory. Set up a 
> break point.

text_poke does not _require_ a break point. text_poke can work with
stop_machine. There are two different problems here :

- How you deal with concurrency
  - you use stop machine
  - I use breakpoints
- How you deal with RO page mappings
  - you change the kernel page flags
  - i use text_poke

Please don't mix those separate concerns.

> Knowing what to do when that break point is hit by another 
> process. Modify the one location. Unmap the page. Free the page. Remove 
> the breakpoint.
> 
> Yes, this may be faster if I only modify one location. I would be hard 
> pressed that this is faster when I modify a few hundred locations. 
> The stop_machine method does it all at once. Not one at a time.
> 
> 
> > 
> > 2 - You claim that using stop_machine is simpler and therefore safer
> > than using a breakpoint-based approach. I start having some doubts about
> > simplicity when you start talking about the workarounds you have to do
> > for NMIs,
> 
> I agree, the NMI work around was tricky, but the final solution (which
> we tested vigorously) works well. My claim that it is simpler is not about 
> the small steps, but rather the number of variables we need to deal with.
> Stop machine shuts down all the CPUs and executes my code on one CPU. 
> Interrupts are disabled on all CPUs, and we only need to worry about the 
> NMI. Which we now do.
> 
> Your solution is about mapping another page on a running system, where
> anything can happen. The number of variables that can go wrong is much 
> greater simply by the fact that you have no idea as to what is running at 
> the same time as you perform your modifications.
> 
> With stop_machine, the number of variables is much less, because I know 
> everything that is happening when I do the modification. I do not need to 
> worry about some strange driver doing some kind of tricks because it 
> simply is not running.
> 
> > but more importantly, you seem to recognise that the latency
> > it induces would be inadequate for production systems.
> 
> Wrong. I recognise the latency of tracing all functions on a production 
> system. Heck, we trace spin_lock, rcu_read_lock, mutex_lock, and all that 
> jazz. Just slowing those functions down a bit will have a noticeable 
> impact. I've found that adding those functions to set_ftrace_notrace drops 
> the function tracer penalty, significantly.
> 
> 
> > Therefore it's
> > unusable in some LTTng use-cases just because of that. If you expect the
> > function tracer to become used more widely in LTTng, these concerns
> > should be addressed.
> 
> If you only want to trace a few hundred functions, then the overhead with
> it on should not be significant. Depending on which functions you trace. 
> As mentioned above, tracing only spin_lock can slow the system down.
> 
> Set up the functions you want to trace, enable them. You can have the
> ring buffer disabled (echo 0 > /debug/tracing/tracing_on) and just turn on 
> the ring buffer for your snapshot, and turn it off when you are done. When 
> all tracing is done, then disable the function tracing.
> 
> 
> > 
> > If, in the end, your argument is "the function tracer works as-is now,
> > and I have no time to change it given it represents too much work" or "I
> > don't care about your use-cases", I'm OK with that. But please then don't
> > argue that it's because it's the best technical solution when it isn't.
> 
> No, I have yet to hear a valuable argument against stop_machine. You are 
> pushing the burden of proof on me, when we have something that does work, 
> on several archs. You want me to redesign the system to be x86 only, and 
> then say, hey, my original code works better.
> 

stop_machine involves high interrupt latency. This is the argument I've
been repeating for 1-2 emails already. And I have to disagree with you :
we can do this code generically given the right abstractions
(BREAKPOINT_INSN* macros I proposed earlier). Is having something that
"works" your only argument to stop improving it ?

> I do not see text_poke being theoretically better. The only reason you 
> given me to use it is because you dislike stop_machine.
> 

There is absolutely no link between stop_machine and text_poke. I argue
against stop_machine saying that the breakpoint approach is less
intrusive because it does not involve disabling interrupts for so long,
and I argue against modifying the kernel page flags because that
modifies the access rights of the core kernel and modules to RO
mappings, which is IMO a side-effect that we should eliminate _if we
can_. Please keep those two concerns separate.

Mathieu

> -- Steve
> 
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
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/

Powered by blists - more mailing lists