On Fri, Oct 24, 2014 at 1:59 PM, Yury Gribov <y.gri...@samsung.com> wrote:
> On 10/24/2014 01:44 PM, Dmitry Vyukov wrote:
>>
>> On Fri, Oct 24, 2014 at 12:28 PM, Yury Gribov <y.gri...@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.
Ok, thank you, now I am on the same page.
Yury, I would expect that the inline instrumentation will become the
default in the kernel as well (it's 2 times faster, or maybe even more
if happens to affect loop registration). Do you agree?
If we consider inline as default, then the user won't be able to
simply switch between abort/noabort with a runtime flag w/o rebuilding
the kernel, because for __asan_report* we have to have 2 versions.
I would also consider that abort/noabort is pretty persistent in a
particular testing environment -- you either use one or another and do
not switch frequently between them.
Taking that into account and the fact that __asan_load* can be emitted
due to call threshold, I am mildly in favor of Jakub's position of
making it all consistent between user/kernel and load/report and
explicit. To make it clear, I mean the runtime interface:
__asan_load* -- not-noreturn, always aborts in runtime on failure
__asan_load*_noabort - not-noreturn, never aborts in runtime on failure
