search

google / kernel-sanitizers

Linux Kernel Sanitizers, fast bug-detectors for the Linux kernel
https://google.github.io/kernel-sanitizers/
437 stars 87 forks source link

Stack & globals instrumentation #27

Closed aryabinin closed 9 years ago

dvyukov commented 9 years ago

If we add a lot of config options, it will be a hell to support all possible configurations. It doesn't look too bad IMO.

Please-please-please remove the configs. Configs are evil all others equal. We already have 8 different configurations. Have somebody tested them at least once? Configs should be added when there is a strong need at hand and no ways around. I do not mind to add them in that situation, but not now. We will get complaints from users about broken why-in-this-world-one-would-ever-use configurations. We will get complaints about missed bugs when a user actually disabled detection long time ago without any particular reason and without understanding what he is doing, it just looked like a "good" idea for him at that time. We should strongly push for a one-configuration-fits-all. And diverge from that only when we understand the actual need (and setup testing for that additional configuration). Rather than adding all possible might-be-useful-in-future configurations proactively.

aryabinin commented 9 years ago

Please-please-please remove the configs. Configs are evil all others equal. We already have 8 different configurations. Have somebody tested them at least once? Configs should be added when there is a strong need at hand and no ways around. I do not mind to add them in that situation, but not now. We will get complaints from users about broken why-in-this-world-one-would-ever-use configurations. We will get complaints about missed bugs when a user actually disabled detection long time ago without any particular reason and without understanding what he is doing, it just looked like a "good" idea for him at that time. We should strongly push for a one-configuration-fits-all. And diverge from that only when we understand the actual need (and setup testing for that additional configuration). Rather than adding all possible might-be-useful-in-future configurations proactively.

That sounds convincing. I'm certainly don't mind of dropping KASAN_GLOBALS config. But I'm not sure how to handle KASAN_STACK, since stack instrumentation won't work with gcc 4.9.2.

Supporting 4.9.2 becoming a bit painful now. I'm not sure that we have a right to drop 4.9.2 support, but this would make our life easier.

aryabinin commented 9 years ago

Updated.

Two pending issues remain:

dvyukov commented 9 years ago

what's wrong with module_free()?

I think I see the point of misunderstanding. I treated kasan_module_load/unload callbacks as platform-independent, and put them into module.c code itself so that all arches do not need to care about it. You made them arch-specific and put into arch-specific code. From this point of view your code is correct.

Is there any reason to move this to arch-specific code? Otherwise keeping it in common code looks better to me.

dvyukov commented 9 years ago

what we gonna do with CONFIG_KASAN_STACK?

How exactly it fails with gcc4.9? I thought that Makefile takes care of removing the unsupported flag, and the compiler just do not emit instrumentation. Which is fine, we just don't catch bugs there. We also map shadow for stack, which can be avoided. But it does not look like a big deal to me. We want people to move to 5.0 because it also has inline instrumentation.

aryabinin commented 9 years ago

Yep, you are right, it shouldn't fail. My concern is - does it worth to keep this option, so 4.9.2 users (do they even exist?) could disable it and save some stack? But seems we don't care much about them, and removing option is preferable, right?

dvyukov commented 9 years ago

But seems we don't care much about them, and removing option is preferable, right?

Yes, it is preferable. There are chances that (1) we will need more modifications to gcc and they won't be in 4.9 or (2) by the time we upstream and polish kasan, gcc 5.0 will be released by distros.

aryabinin commented 9 years ago

Removed stack config