Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: SIGILL caused by rdcycle prohibited by kernel on RISC-V #10901

Closed
wants to merge 1 commit into from

Conversation

XieJiSS
Copy link
Contributor

@XieJiSS XieJiSS commented Oct 29, 2022

Hello,

This issue is probably caused by the PMU driver explicitly disallowing userland access to the mcycle register via rdcycle/rdcycleh.

Let me clarify a little bit:

  1. Someone (e.g. those who write the PMU drivers) believe that mcycle is too accurate that might leak info to side-channel attacks, so they disabled userland access to mcycle by adding a commit to the kernel. As a result, rdcycle causes SIGILL now. We can observe this behavior happening on SiFive Unmatched boards.
  2. Regarding this, the Google Highway library has changed from rdcycle to rdtime. However, the Google Benchmark library remains unchanged (as of now, 20221030).
  3. The kernel devs had noticed this userland breakage, and had submitted a patch to re-enable using rdcycle in userland, which was applied 17 days ago. Since this patch is only accepted recently, I hope it can land in linux 6.2? As of our version of linux, which is linux 6.0.2, rdcycle is not working yet.

So, pros of switching to rdtime:

  1. Runtime SIGILLs disappear immediately
  2. Probably reducing the attack surface from side-channel attacks? Not sure tho
  3. mtime register is promised to increase at a fixed speed, while mcycle's increment speed may vary. See comments by Anup in this link.
  4. rdcycle is now part of the Zicntr extension, which is not yet ratified as of 2022-10. RISC-V, despite used by a growing group of users, is still an evolving ISA, so details of rdcycle may change in the future. Should we consider this at software level? Sounds more like something that compilers should worry about.

And cons:

  1. We can expect the kernel releasing new versions that re-enables rdcycle.
  2. Seems like rdcycle has a higher resolution.

FYI: The corresponding code is introduced by me in #9215.

See-also this thread: http://lists.infradead.org/pipermail/linux-riscv/2022-September/018862.html

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Oct 29, 2022

CC-ing @adamretter : I'm not sure whether your SSH access to our Unmatched board still remains or not X(

Let me know if you have difficulties logging-in to the board. Thanks!

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Oct 29, 2022

CI build-linux-clang10-asan failed because of [ FAILED ] BlobDBTest.FilterForFIFOEviction (180 ms), which appears to be unrelated to this PR :-P

@adamretter
Copy link
Collaborator

adamretter commented Oct 31, 2022

@XieJiSS Yes, I am still able to access the RISC-V machine. I have added this one into our backlog here at EB and prioritised it (cc @alanpaxton)

@adamretter adamretter added the RiscV64 Build or run RocksDB on Risc label Oct 31, 2022
@alanpaxton
Copy link
Contributor

Hi @XieJiSS - Adam has set me up on the RISC-V box and I have got some code built, but I'm struggling to understand what I need to run to reproduce the problem in the unfixed main, and hence verify the fix in your PR. I'm not really clear where toku_time gets used in rocksdb ?

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Nov 3, 2022

Hi @alanpaxton, personally I'm not really sure about where toku_time is used, but toku_time missing RISC-V implementation caused the build process to fail. That's why I created PR #9215.

In short: I used rdcycle in that PR, which used to work, as Adam and I had tested & confirmed before. A few days ago, when fixing a python bug, I noticed that rdcycle stopped working, generating a SIGILL instead. So I immediately realized that PR #9215 is also affected, which lead to the current PR.

The original proposed implementation (the rdcycle-based one) is cherry-picked from the Google benchmark library. The fix in this PR is derived from this Google highway library commit. Hope these information helps ;-)

Some screenshots of my test result:

image

image

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Nov 3, 2022

Just as a reminder - this would be fixed automatically in (not yet released) newer Linux versions, and the kernel patch are expected to be backported to previous versions. I've listed pros and cons about switching from rdcycle to rdtime in the description of this PR, and it is up to the maintainers to decide whether this switch is necessary or not.

Thanks!

@alanpaxton
Copy link
Contributor

Hi @XieJiSS thanks for the explanation. I can build on the RISC-V box - main has your build PR applied, and I needed a -latomic somewhere, which I'll PR later. But what do you run to cause a SIGILL ? It does not happen for me running db_basic_test for instance. (make clean; make db_basic_test; ./db_basic_test) works.

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Nov 3, 2022

If rdcycle didn't trigger an SIGILL, then you are probably on an older version of the kernel - consider that the RISC-V box is a Ubuntu, this is highly possible

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Nov 3, 2022

IIRC maybe you can try to setup an Arch Linux container to test on the latest kernel: https://github.com/felixonmars/archriscv-packages/wiki/Setup-Arch-Linux-RISC-V-Development-Environment

EDIT: Seems like systemd-nspawn works as syscall level and hence does not simulate a new kernel. I'll contact you later and let's see whether I can help you get access to some Arch Linux dev boards :)

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Nov 17, 2022

Sorry for the lag. Arch Linux board resources are quite tight these days, but I managed to start a qemu-system with Arch Linux rootfs + linux 6.0.x kernel, which IMO is suitable for debugging & testing of this problem. I'll provide a tutorial ASAP.

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Jan 4, 2023

Sorry for the lag. Been overloaded these days, and we also discovered several annoying bugs in our qemu-system setup script... but anyway, the tutorial about creating a qemu-system (which simulates a kernel) is here, and the qemu-user (which does not simulate the kernel) version can be found here.

@alanpaxton
Copy link
Contributor

Thanks for doing that. I have a couple of things to finish off atm but I plan to get back to this in the near future.

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Jan 4, 2023

BTW my colleague also managed to get a new RISC-V board, but he's been caught by COVID. Probably after the Spring Festival (lunar new year) he can get back to work and flash the board so that we can access it via SSH to do some development or testing.

@alanpaxton Can I use https://github.com/alanpaxton.keys to set up your account's SSH credentials? Also cc @adamretter seems like the original board we used last year needs to be reset & move to another city physically, for some other usage, so we won't be able to access it anymore in near future. Do you need an account on the new board?

@alanpaxton
Copy link
Contributor

@XieJiSS yes, those are current ssh keys

@adamretter
Copy link
Collaborator

@XieJiSS Is this still an issue for you? We have build of RocksJava for RISC-V now and have access to both emulated and physical RISC-V hardware.

@XieJiSS
Copy link
Contributor Author

XieJiSS commented Jan 17, 2024

@XieJiSS Is this still an issue for you? We have build of RocksJava for RISC-V now and have access to both emulated and physical RISC-V hardware.

I think QEMU has recently released a version with emulated /proc/cpuinfo in usermode, and the kernel has reverted the offending commit which causes rdcycle to SIGILL. So maybe we can close this for now?

@ajkr ajkr closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed RiscV64 Build or run RocksDB on Risc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants