Introduce KASAN support for the riscv architecture. The implementation is similar to arm64.
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 74518 Build 71401: arc lint + arc unit
Event Timeline
| contrib/llvm-project/clang/lib/Driver/ToolChains/FreeBSD.cpp | ||
|---|---|---|
| 485 | Please create a pull request on llvm upstream and add the LLVM PR number to the commit message and summary. | |
| sys/conf/kern.mk | ||
| 276–277 | ||
| sys/riscv/include/asan.h | ||
| 5 | why?
| |
Prevent KASAN from intercepting atomic functions that are unsupported by the
architecture. The specific changes can be summarized as follows:
- Use ARCH_SUPPORT_ATOMIC_*_WIDTH x-macros to control the generation of interceptor functions in subr_asan.c.
- Use #undef in machine/atomic.h to undefine the macros that are forcibly defined by sys/atomic_san.h but lack actual MD implementations.
Please let me know if there is a better way to implement this.
| sys/riscv/include/asan.h | ||
|---|---|---|
| 5 | Oh nvm, this is from sys/arm64/asan.h. Sorry for my mistake, you can mark these as resolved. | |
| sys/kern/subr_asan.c | ||
|---|---|---|
| 655 ↗ | (On Diff #179088) | These lines are too long and should be wrapped to 80 columns. |
| 831 ↗ | (On Diff #179088) | Could you please explain what you are changing in this file? The review description does not say anything, and this clearly isn't riscv-specific. |
| sys/riscv/include/atomic.h | ||
| 90 | Again, what's happening here? | |
| sys/riscv/riscv/pmap.c | ||
| 5521 | This is missing a comment explaining where the constants come from. | |
| 5581 | Indentation here is wrong, see style(9). | |
| 5597 | Indentation here is wrong. | |
| 5663 | Why wmb? Shouldn't all of these routines end with sfence_vma()? | |
| sys/kern/subr_asan.c | ||
|---|---|---|
| 831 ↗ | (On Diff #179088) | Sorry, I should have provided more context in the review description. Here is the background:
I initially tried adding these missing atomic implementations for riscv in D57379. However, jrtc27 pointed out that this could introduce performance regressions, and that the correct approach is to just have KASAN not intercept atomic functions that aren't implemented by the architecture. Therefore, my implementation logic here is:
c #ifdef atomic_add_8 ASAN_ATOMIC_FUNC_ADD(8, uint8_t); #endif However, this would lead to massive file bloat and fails to handle some cases (e.g., when atomic_* exists but atomic_*_acq does not). I'm not entirely sure if this is the most elegant solution, but it is the best method I could come up with so far. |
| sys/riscv/include/atomic.h | ||
| 90 | As explained above, the MI part defaults to assuming all atomic operations are supported. This MD section here overrides the width declarations for the operations that are not fully supported. This way, we don't need to modify the code for amd64 and arm64, since they already support almost all atomic operations. | |
Hi, glad to see this review.
I will take a detailed look next week, and I hope to test it out as well.
Fixed hart boot hang on riscv smp caused by KASAN, by initializing the tp register earlier in the assembly stage.
Hi. This is in great shape.
I tested in QEMU (both Sv39 and Sv48 mode), and on the VisionFive v2 and Allwinner D1 hardware. In fact, it detected a real bug on the latter!
I reviewed the pmap and other riscv machine-dependent code; those early functions are tricky but this looks clean enough.
I did not look in detail at the changes you made to these interceptor declarations. Maybe @markj can weigh in there.
A few small comments inline, but otherwise LGTM.
| sys/conf/kern.mk | ||
|---|---|---|
| 277–283 | What is TODO now? | |
| sys/riscv/riscv/pmap.c | ||
| 897 | Please add a comment mentioning pmap_san_enter_early() as what creates these mappings. Otherwise this is quite obscure compared to the surrounding code. | |
| 1060–1089 | I suggest retaining some of the detailed comments from the arm64 version. | |
| 5628–5631 | ||
| 5633 | ||
| 5638–5639 | ||
| 5670 | For a long #ifdef block, it is good to indicate what condition we are closing. | |
@zishun.yi.dev_gmail.com could you please rebase this patch so that it applies to the latest main? I don't really like the diff to riscv/include/atomic.h, and would like to explore different approaches. (Really I'd like to get rid of all of these interceptors entirely, but the best way to do that is probably to reimplement atomic.h using C11 atomics...)
- Address mhorne@'s comments, thanks for testing, and it's great to know that KASAN already caught a real bug!
- Rebase to origin/main
- Move the MI interceptor code to a separate parent revision.
Yeah, I agree the current code is ugly, and your approach sounds great.
For now I've moved most of the ugly code into a separate revision(D58037) so it doesn't block the riscv work. That part could well be refactored entirely in the future , so keeping it separate from the KASAN changes seems cleaner.
| sys/conf/kern.mk | ||
|---|---|---|
| 277–283 | Similar to https://reviews.llvm.org/D98285, we need to upstream the value of -asan-mapping-offset for riscv | |