Re: Instrumentation and RCU

[Thomas Gleixner]

Masami,

Masami Hiramatsu <mhiramat@xxxxxxxxxx> writes:
> On Mon, 09 Mar 2020 19:59:18 +0100
> Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
>> >> #2) Breakpoint utilization
>> >> 
>> >>     As recent findings have shown, breakpoint utilization needs to be
>> >>     extremly careful about not creating infinite breakpoint recursions.
>> >> 
>> >>     I think that's pretty much obvious, but falls into the overall
>> >>     question of how to protect callchains.
>> >
>> > This is rather unique, and I agree that its best to at least get to a point
>> > where we limit the tracing within breakpoint code. I'm fine with making
>> > rcu_nmi_exit() nokprobe too.
>> 
>> Yes, the break point stuff is unique, but it has nicely demonstrated how
>> much of the code is affected by it.
>
> I see. I had followed the callchain several times, and always found new function.
> So I agree with the off-limit section idea. That is a kind of entry code section
> but more generic one. It is natural to split such sensitive code in different
> place.
>
> BTW, what about kdb stuffs? (+Cc Jason)

That's yet another area of wreckage which nobody every looked at.

>> >> #4 Protecting call chains
>> >> 
>> >>    Our current approach of annotating functions with notrace/noprobe is
>> >>    pretty much broken.
>> >> 
>> >>    Functions which are marked NOPROBE or notrace call out into functions
>> >>    which are not marked and while this might be ok, there are enough
>> >>    places where it is not. But we have no way to verify that.
>
> Agreed. That's the reason why I haven't add kprobe-fuzzer yet.
> It is easy to make a fuzzer for kprobes by ftrace (note that we need
> to enable CONFIG_KPROBE_EVENTS_ON_NOTRACE=y to check notrace functions),
> but there is no way to kick the target code. In the result, most of the
> kprobed functions are just not hit. I'm not sure such test code is
> reasonable or not.

Well, test code is always reasonable, but you have to be aware that code
coverage is a really hard to solve problem with a code base as complex
as the kernel.

>> which is also in section "text" then the analysis tool will find the
>> missing offlimit_safecall() - or what ever method we chose to annotate
>> that stuff. Surely not an annotation on the called function itself
>> because that might be safe to call in one context but not in another.
>
> Hmm, what the offlimit_safecall() does? and what happen if the 
> do_fragile_stuff_on_enter() invokes a library code? I think we also need
> to tweak kbuild to duplicate some library code to the off-limit text
> area.

That's why we want the sections and the annotation. If something calls
out of a noinstr section into a regular text section and the call is not
annotated at the call site, then objtool can complain and tell you. What
Peter and I came up with looks like this:

noinstr foo()
	do_protected(); <- Safe because in the noinstr section

	instr_begin();	<- Marks the begin of a safe region, ignored
        		   by objtool

        do_stuff();     <- All good   

        instr_end();    <- End of the safe region. objtool starts
			   looking again

        do_other_stuff();  <- Unsafe because do_other_stuff() is
        		      not protected
and:

noinstr do_protected()
        bar();		<- objtool will complain here

See?

>> These annotations are halfways easy to monitor for abuse and they should
>> be prominent enough in the code that at least for the people dealing
>> with that kind of code they act as a warning flag.
>
> This off-limit text will be good for entries, but I think we still not
> able to remove all NOKPROBE_SYMBOLS with this.
>
> For example __die() is marked a NOKPROBE because if we hit a recursive
> int3, it calls BUG() to dump stacks etc for debug. So that function
> must NOT probed. (I think we also should mark all backtrace functions
> in this case, but not yet) Would we move those backtrace related
> functions (including printk, and console drivers?) into the offlimit
> text too?

That's something we need to figure out and decide on. Some of this stuff
sureley wants to be in the noinstr section. Other things might end up
still being explicitely annotated, but that should be the exception not
the rule.

> Hmm, if there is a bust_kprobes(), that can be easy to fix this issue.

That might help, but is obviously racy as hell.

Thanks,

        tglx