Re: [PATCHv4] Enable -fsanitize-recover for KASan

[Yury Gribov <y dot gribov at samsung dot com>]

On 10/24/2014 01:44 PM, Dmitry Vyukov wrote:
> On Fri, Oct 24, 2014 at 12:28 PM, Yury Gribov <y.gribov@samsung.com> wrote:
> > On 10/23/2014 03:10 PM, Andrey Ryabinin wrote:
> > > On 10/23/2014 02:38 PM, Jakub Jelinek wrote:
> > > > On Thu, Oct 23, 2014 at 02:33:42PM +0400, Yury Gribov wrote:
> > > > > Actually this is a historical artifact.  If inlining proves to be
> > > > > significantly faster, they may want to switch.
> > > >
> > > >
> > > > Ok.
> > > >
> > > > > > So, at that point you can include your ugly hacks in __asan_load* logic
> > > > > > in
> > > > > > the kernel, the difference between __asan_load4 and
> > > > > > __asan_load4_noabort
> > > > > > will be just that the latter will always return, while the former will
> > > > > > not
> > > > > > if some error has been reported.
> > > > > > All the __asan_load* and __asan_store* entrypoints, regardless of
> > > > > > -f{,no-}sanitize-recover=kernel-address are by definition not noreturn,
> > > > > > they
> > > > > > in the common case (if the code is not buggy) return.
> > > > >
> > > > >
> > > > > Perhaps we should just keep __asan_load* as is and leave the decision
> > > > > whether to abort or continue for the runtime?  This would make semantics
> > > > > of
> > > > > -fsanitize-recover cumbersome though (because it wouldn't work if user
> > > > > selects outline instrumentation).
> > > >
> > > >
> > > > Well, the "don't ever report anything while some per-CPU flag is set"
> > > > thing
> > > > can be considered as part of the "is this memory access ok" test, it is
> > > > pretending everything is accessible.
> > > >
> > > > But, otherwise, if it is supposed to be developer's decision at compile
> > > > time, __asan_load*_noabort should better always continue, even if it
> > > > reported issues, and __asan_load* should better not return after
> > > > reporting
> > > > errors.
> > >
> > > True, but why we need new functions for that.
> > > __asan_load could also abort or not depending on what user/developer
> > > wants.
> > > Why we have to rebuild the entire kernel if someone wants to switch from
> > > abort to noabort?
> > >
> > > I'm not against __asan_load_noabort, I'm just saying that this is no point
> > > to have separate
> > > __asan_load/__asan_load_noabort functions in kernel.
> >
> >
> > I'd still suggest to emit __asan_load_noabort so that we match userspace
> > (where __asan_load strictly matches __asan_report in terminating the
> > program).  Behavior of __asan_load_noabort can further be restricted by user
> > via various environment settings (kernel parameters, /proc, etc.).
> >
> > @Dmitry: what's your opinion on this?
>
> I am somewhat lost in this thread and probably missing something.
> But why do we need __asan_load (which is not noabort) at all? Outline
> instrumentation is non a default mode for both user-space asan and
> kasan (at least in the envisioned future).

Well, it's still enabled automatically if number of memory accesses is 
getting large enough (I think it was 7000?) so it is default in a way.

> I would expect that these
> non-typical cases that use outline instrumentation can also bear the
> overhead of non-noreturn functions.

As Jakub mentioned both __asan_load and __asan_load_noabort will _not_ 
be NORETURN.  Two different versions are necessary so that compiler can 
insert outline instrumentation that matches the -fsanitize-recover setting.

> Can we use just one version of
> __asan_load and let runtime decide on abort?

In this case semantics of inline and outline instrumentation will differ 
(the former depending on -fsanitize-recover compile-time setting and the 
latter depending on runtime options) which may be undesirable given that 
compiler may automatically choose to switch from inline to outline 
depending on function size.

-Y