Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200605082839.226418-1-elver@google.com> <CACT4Y+ZqdZD0YsPHf8UFJT94yq5KGgbDOXSiJYS0+pjgYDsx+A@mail.gmail.com>
 <20200605120352.GJ3976@hirez.programming.kicks-ass.net>
In-Reply-To: <20200605120352.GJ3976@hirez.programming.kicks-ass.net>
From:   Andrey Konovalov <andreyknvl@google.com>
Date:   Fri, 5 Jun 2020 15:25:34 +0200
Message-ID: <CAAeHK+zErjaB64bTRqjH3qHyo9QstDSHWiMxqvmNYwfPDWSuXQ@mail.gmail.com>
Subject: Re: [PATCH -tip v3 1/2] kcov: Make runtime functions noinstr-compatible
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Dmitry Vyukov <dvyukov@google.com>, Marco Elver <elver@google.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Borislav Petkov <bp@alien8.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>,
        clang-built-linux <clang-built-linux@googlegroups.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Alexander Potapenko <glider@google.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 5, 2020 at 2:04 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Jun 05, 2020 at 12:57:15PM +0200, Dmitry Vyukov wrote:
> > On Fri, Jun 5, 2020 at 10:28 AM Marco Elver <elver@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.

To clarify this part AFAIU it, even if we try to prefault the whole
remote area each time kcov_remote_start() is called, then (let alone
the performance impact) the kernel thread can be rescheduled between
kcov_remote_start() and kcov_remote_stop(), and then it might be
running with a different mm than the one that was used when
kcov_remote_start() happened.