Page MenuHomeFreeBSD

riscv: add KASAN support
Needs ReviewPublic

Authored by zishun.yi.dev_gmail.com on Jun 1 2026, 10:16 AM.
Tags
None
Referenced Files
F161537451: D57381.diff
Sat, Jul 4, 3:42 PM
F161465922: D57381.diff
Sat, Jul 4, 2:04 AM
Unknown Object (File)
Thu, Jul 2, 2:42 PM
Unknown Object (File)
Sat, Jun 27, 4:03 PM
Unknown Object (File)
Wed, Jun 24, 8:57 PM
Unknown Object (File)
Tue, Jun 23, 4:14 PM
Unknown Object (File)
Sat, Jun 20, 12:15 PM
Unknown Object (File)
Fri, Jun 19, 10:47 PM

Details

Reviewers
mhorne
markj
Group Reviewers
riscv
Summary

Introduce KASAN support for the riscv architecture. The implementation is similar to arm64.

LLVM PR: https://github.com/llvm/llvm-project/pull/202288

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

minsoochoo0122_proton.me added inline comments.
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

This has been resolved since LLVM 20 (commit) and we recently MFVed LLVM 21. We can remove this line now. ping @dim

sys/riscv/include/asan.h
5

why?

  1. the year is wrong
  2. we changed preferred license format (see licensing policy)

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:

  • riscv lacks many sub-word atomic implementations (at least prior to the Zabha extension).
  • Currently, atomic_san.h unconditionally adds atomic defines across all architectures to intercept these operations, even if an architecture like riscv doesn't actually support some of them.
  • Similarly, subr_kasan.c forces the implementation of all atomic interceptor functions for all architectures.

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:

  • For declarations: machine/atomic.h should #undef the operations it does not support.
  • For interceptor generation (subr_kasan.c): One alternative was to wrap every generation site in an #ifdef
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).
To resolve this, I used X-macros. For example, use ARCH_SUPPORT_ATOMIC_ADD_WIDTH to declare the atomic_add width supported by the architecture.
The MI part defaults to supporting all atomic operations, while the MD part overrides the ones it lacks.

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.

Hi, glad to see this review.

I will take a detailed look next week, and I hope to test it out as well.

Thanks for taking a look :)

Fixed hart boot hang on riscv smp caused by KASAN, by initializing the tp register earlier in the assembly stage.

zishun.yi.dev_gmail.com marked 3 inline comments as done.

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.

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!

See D57951. It took me a while to determine that it wasn't a false report.

@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...)

zishun.yi.dev_gmail.com marked 6 inline comments as done.
  • 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.

@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...)

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