On Fri, Jun 05, 2020 at 12:57:15PM +0200, Dmitry Vyukov wrote: > On Fri, Jun 5, 2020 at 10:28 AM Marco Elver <el...@google.com> wrote: > > > > While we lack a compiler attribute to add to noinstr that would disable > > KCOV, make the KCOV runtime functions return if the caller is in a > > noinstr section, and mark them noinstr. > > > > Declare write_comp_data() as __always_inline to ensure it is inlined, > > which also reduces stack usage and removes one extra call from the > > fast-path. > > > > In future, our compilers may provide an attribute to implement > > __no_sanitize_coverage, which can then be added to noinstr, and the > > checks added in this patch can be guarded by an #ifdef checking if the > > compiler has such an attribute or not. > > Adding noinstr attribute to instrumentation callbacks looks fine to me. > > But I don't understand the within_noinstr_section part. > As the cover letter mentions, kcov callbacks don't do much and we > already have it inserted and called. What is the benefit of bailing > out a bit earlier rather than letting it run to completion? > Is the only reason for potential faults on access to the vmalloc-ed > region?
Vmalloc faults (on x86, the only arch that had them IIRC) are gone, per this merge window. The reason I mentioned them is because it is important that they are gone, and that this hard relies on them being gone, and the patch didn't call that out. There is one additional issue though; you can set hardware breakpoint on vmalloc space, and that would trigger #DB and then we'd be dead when we were already in #DB (IST recursion FTW). And that is not something you can trivially fix, because you can set the breakpoint before the allocation (or perhaps on a previous allocation). That said; we already have this problem with task_struct (and task_stack). IIRC Andy wants to fix the task_stack issue by making all of noinstr run on the entry stack, but we're not there yet. There are no good proposals for random allocations like task_struct or in your case kcov_area. > Andrey, Mark, do you know if it's possible to pre-fault these areas? Under the assumption that vmalloc faults are still a thing: You cannot pre-fault the remote area thing, kernel threads use the mm of the previous user task, and there is no guarantee that mm will have had the vmalloc fault.
