Introduce KASAN support for the riscv architecture. The implementation is similar to arm64.
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 73732 Build 70615: 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?
| |
Comment Actions
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 | These lines are too long and should be wrapped to 80 columns. | |
| 890 | 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 | ||
| 5492 | This is missing a comment explaining where the constants come from. | |
| 5552 | Indentation here is wrong, see style(9). | |
| 5568 | Indentation here is wrong. | |
| 5634 | Why wmb? Shouldn't all of these routines end with sfence_vma()? | |
| sys/kern/subr_asan.c | ||
|---|---|---|
| 890 | 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. | |
Comment Actions
Hi, glad to see this review.
I will take a detailed look next week, and I hope to test it out as well.
Comment Actions
Fixed hart boot hang on riscv smp caused by KASAN, by initializing the tp register earlier in the assembly stage.
Comment Actions
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 | 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. | |
| 1059–1088 | I suggest retaining some of the detailed comments from the arm64 version. | |
| 5599–5602 | ||
| 5604 | ||
| 5609–5610 | ||
| 5641 | For a long #ifdef block, it is good to indicate what condition we are closing. | |
Comment Actions
@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...)
Comment Actions
- 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.
Comment Actions
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 | Similar to https://reviews.llvm.org/D98285, we need to upstream the value of -asan-mapping-offset for riscv | |
Comment Actions
I finally got back to this, sorry for the delay. I wrote some smaller patches that are sufficient to deal with the atomic issues: https://github.com/markjdb/freebsd/commits/main-riscv-kasan/
The change to subr_asan.c is a bit ugly, but I think it's acceptable, and if we can reimplement atomic(9) on top of C11 atomics most of that code will go away anyway. The atomic_subword.h change seems like a nice cleanup on its own, I submitted for review already.
What do you think?
Comment Actions
Yeah, I think the change to subr_asan.c is acceptable. And the other commits seem clean.
and if we can reimplement atomic(9) on top of C11 atomics most of that code will go away anyway.
If no one is working on this, I can look into it to see if it's feasible, and then give it a try : )
Sorry, I just realized I forgot to submit this comment earlier.
Comment Actions
I submitted D58680. Once that lands in some form, you can fold the other commits from that branch into your patch, and I think it'll be good to go.
and if we can reimplement atomic(9) on top of C11 atomics most of that code will go away anyway.
If no one is working on this, I can look into it to see if it's feasible, and then give it a try : )
As far as I know no one is working on this. @jhb and I have talked about it a few times is all (that I'm aware of).
To be clear, this is not really a straightforward thing. To start, I'd add a kernel config option, options C11_ATOMICS or something, and provide a sys/ header which overrides machdep/atomic.h when this option is configured. That is, we'd use C11 atomics to implement the atomic(9) interface; converting all of the kernel away from atomic(9) would be a ton of work. Then, there are some considerations:
- How do we handle userspace components which rely on atomic(9)? Can they use C11 atomics or do we have to provide the existing implementations indefinitely in some form? Is there anything in the ports tree which uses atomic(9)?
- How are arm64 LSE atomics handled? Right now we check at boot time whether the CPUs implement them, and if so set a flag so that atomic(9) dispatches everything to the LSE implementation (which is significantly more performant on some platforms). I have no idea how compilers handle LSE in their builtins.
- Do we have requisite compiler support on all of our architectures? I'd expect so, but I'm very much not a compiler person and don't have much intuition here.
- What about gcc?
- If options C11_ATOMICS is configured, how does the generated machine code compare with what we get today? Are there any anomalies, e.g., in performance, .text size, support in older compilers?
- ... probably a bunch of other issues that haven't occurred to me
I do think this is all worth pursuing, just warning it might be challenging to resolve some of these questions.
Comment Actions
Sounds great. Thank you so much for writing those commits to help with this! I will fold them into my patch once D58680 lands.
and if we can reimplement atomic(9) on top of C11 atomics most of that code will go away anyway.
If no one is working on this, I can look into it to see if it's feasible, and then give it a try : )
As far as I know no one is working on this. @jhb and I have talked about it a few times is all (that I'm aware of).
To be clear, this is not really a straightforward thing. To start, I'd add a kernel config option, options C11_ATOMICS or something, and provide a sys/ header which overrides machdep/atomic.h when this option is configured. That is, we'd use C11 atomics to implement the atomic(9) interface; converting all of the kernel away from atomic(9) would be a ton of work. Then, there are some considerations:
- How do we handle userspace components which rely on atomic(9)? Can they use C11 atomics or do we have to provide the existing implementations indefinitely in some form? Is there anything in the ports tree which uses atomic(9)?
- How are arm64 LSE atomics handled? Right now we check at boot time whether the CPUs implement them, and if so set a flag so that atomic(9) dispatches everything to the LSE implementation (which is significantly more performant on some platforms). I have no idea how compilers handle LSE in their builtins.
- Do we have requisite compiler support on all of our architectures? I'd expect so, but I'm very much not a compiler person and don't have much intuition here.
- What about gcc?
- If options C11_ATOMICS is configured, how does the generated machine code compare with what we get today? Are there any anomalies, e.g., in performance, .text size, support in older compilers?
- ... probably a bunch of other issues that haven't occurred to me
I do think this is all worth pursuing, just warning it might be challenging to resolve some of these questions.
And thanks for listing out those challenges. I'll use them as a guide and start looking into it as a side project.
Comment Actions
Looks good! Thanks for your patience with getting the atomics sorted out.
@mhorne will you land this? I'm happy to if you're busy.
Comment Actions
Please go ahead, I have some other patches in my queue that need handling.
I suggest splitting the LLVM piece into a separate commit, is all.
Comment Actions
Hmm, I missed that the LLVM change has not yet landed. @zishun.yi.dev_gmail.com will you add some tests as requested in the PR? If that's done, I wonder if @dim or @aokblast can help land it?
Comment Actions
I just leave coments on the upstream PR. I can help to merge upstream. We need to MFC from upstream so that this patch should not contains the LLVM change.