Bug 214055
| Summary: | KASAN: add atomic tests | ||
|---|---|---|---|
| Product: | Memory Management | Reporter: | Andrey Konovalov (andreyknvl) |
| Component: | Sanitizers | Assignee: | MM/Sanitizers virtual assignee (mm_sanitizers) |
| Status: | RESOLVED CODE_FIX | ||
| Severity: | normal | CC: | kasan-dev, melver, paul.heidekrueger |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | upstream | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
| Attachments: | [PATCH RFC] kasan: add atomic tests | ||
|
Description
Andrey Konovalov
2021-08-12 13:00:59 UTC
FYI, I'm looking into this. Created attachment 305788 [details]
[PATCH RFC] kasan: add atomic tests
Here's a first draft for implementing kasan tests. Is this along the lines of what you had in mind?
I am unsure as to how extensive the tests should be. For instance, do I need to differentiate between the different KASan modes here?
What do you think?
Many thanks,
Paul
> [PATCH RFC] kasan: add atomic tests
Looks pretty good. Some comments:
- It's unclear this test will work for tag-based KASAN (CONFIG_KASAN_SW_TAGS and CONFIG_KASAN_HW_TAGS, i.e. Arm TBI or MTE based KASAN). If you can't verify that (e.g. by building an arm64 KASAN_SW_TAGS kernel and booting it in qemu), add the KASAN_TEST_NEEDS_CONFIG_ON(...) like in the kasan_bitops_generic() test. In that case, you also might want to rename the test to kasan_atomics_generic(), since it only works in KASAN generic mode (and is untested for tags modes).
- I think there's no need to say in code comments it's based on kasan_bitops_generic(). Instead maybe just move it closer to that test (perhaps right under the last bitops test), so that the file remains (relatively) organized.
Thanks,
-- Marco
Looks like a good start! For the operations that touch two memory areas, we should check that KASAN detects bad accesses through both pointers. I.e. call the operation twice, both times with one good and one bad address. Please also add tests for READ_ONCE, WRITE_ONCE, smp_load_acquire, and smp_store_release (is there something else like that?). There are also atomic64_* and other atomic operations not prefixed with atomic_ listed in include/linux/atomic/atomic-instrumented.h, but I'm not sure if it makes sense to add tests for them, as there's quite a lot of them. I also second Marco's comments. I expect that the test should work as is with the tag-based modes without issues. Please mail the patch on the list - it'll be easier to review it there. Thanks! Resolved with [1] by Paul. Thank you! [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4e76c8cc3378a20923965e3345f40f6b8ae0bdba |