On Thu, 4 Jun 2020 at 13:09, Peter Zijlstra <pet...@infradead.org> wrote: > > On Thu, Jun 04, 2020 at 11:50:57AM +0200, Marco Elver wrote: > > The KCOV runtime is very minimal, only updating a field in 'current', > > and none of __sanitizer_cov-functions generates reports nor calls any > > other external functions. > > Not quite true; it writes to t->kcov_area, and we need to make > absolutely sure that doesn't take faults or triggers anything else > untowards.
Ah, right. > > Therefore we can make the KCOV runtime noinstr-compatible by: > > > > 1. always-inlining internal functions and marking > > __sanitizer_cov-functions noinstr. The function write_comp_data() is > > now guaranteed to be inlined into __sanitize_cov_trace_*cmp() > > functions, which saves a call in the fast-path and reduces stack > > pressure due to the first argument being a constant. > > > > 2. For Clang, correctly pass -fno-stack-protector via a separate > > cc-option, as -fno-conserve-stack does not exist on Clang. > > > > The major benefit compared to adding another attribute to 'noinstr' to > > not collect coverage information, is that we retain coverage visibility > > in noinstr functions. We also currently lack such an attribute in both > > GCC and Clang. > > > > > -static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip) > > +static __always_inline void write_comp_data(u64 type, u64 arg1, u64 arg2, > > u64 ip) > > { > > struct task_struct *t; > > u64 *area; > > @@ -231,59 +231,59 @@ static void notrace write_comp_data(u64 type, u64 > > arg1, u64 arg2, u64 ip) > > } > > } > > This thing; that appears to be the meat of it, right? > > I can't find where t->kcov_area comes from.. is that always > kcov_mmap()'s vmalloc_user() ? Yeah, looks like it. > That whole kcov_remote stuff confuses me. > > KCOV_ENABLE() has kcov_fault_in_area(), which supposedly takes the > vmalloc faults for the current task, but who does it for the remote? > > Now, luckily Joerg went and ripped out the vmalloc faults, let me check > where those patches are... w00t, they're upstream in this merge window. > > So no #PF from writing to t->kcov_area then, under the assumption that > the vmalloc_user() is the only allocation site. > > But then there's hardware watchpoints, if someone goes and sets a data > watchpoint in the kcov_area we're screwed. Nothing actively prevents > that from happening. Then again, the same is currently true for much of > current :/ > > Also, I think you need __always_inline on kaslr_offset() > > > And, unrelated to this patch in specific, I suppose I'm going to have to > extend objtool to look for data that is used from noinstr, to make sure > we exclude it from inspection and stuff, like that kaslr offset crud for > example. > > Anyway, yes, it appears you're lucky (for having Joerg remove vmalloc > faults) and this mostly should work as is. Hmm, looks like this doesn't generally work then. :-/ An alternative would be to check if '__noinstr_text_start <= _RET_IP_ < __noinstr_text_end' in __sanitizer_cov-functions and return if that's the case. This could be #ifdef'd when we get a compiler that can do __no_sanitize_coverage. At least that way we get working KCOV for now. Would that work? Thanks, -- Marco