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

A few improvements for riscv #5662

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

AlexGhiti
Copy link

This reenables KASAN_INLINE and sets 4-level page table by default: note that we can use a kernel command line parameter instead if you prefer ("no5lvl").

Copy link

google-cla bot commented Jan 9, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@a-nogikh
Copy link
Collaborator

a-nogikh commented Jan 9, 2025

Thanks!

For the dashboard/config/linux/bits: reenable KASAN_INLINE for riscv64 commit, you also need to regenerate the configs:

To update kernel configs:
- change config fragments as necessary (e.g. add additional configs to [subsystems.yml](bits/subsystems.yml) along
with minimal kernel version)
- run `syz-env make configs SOURCEDIR=/path/to/existing/linux/checkout`
(note: it will be mrproper-ed and a number of remotes will be added)
(see [this](/docs/contributing.md#using-syz-env) on how to setup/use `syz-env`)
- check in config fragments and changed kernel configs and send a PR
- changes will be deployed to `syzbot` within a day after merging

I.e. run ./tools/syz-env make configs SOURCEDIR=/path/to/linux/checkout and commit the changes.

And regarding vm/qemu: Run riscv64 kernel using 4-level page table: CI checks complain about the capitalization of Run, it should have been titled vm/qemu: run riscv64 kernel using 4-level page table.

@AlexGhiti
Copy link
Author

Arf, sorry about the "Run"...

Since I'm here, I noticed riscv is excluded from a few configs in files like dashboard/config/linux/bits/subsystems.yml although we implement them and they appear in the final generated config. Is there something to do here? Not sure I understood the goal of those.

Thanks for your quick answer!

@a-nogikh
Copy link
Collaborator

a-nogikh commented Jan 9, 2025

I noticed riscv is excluded from a few configs in files like dashboard/config/linux/bits/subsystems.yml although we implement them and they appear in the final generated config. Is there something to do here? Not sure I understood the goal of those.

Overall, we try to reduce the number of enabled configs for the platforms that we run in emulation (like riscv) - the execution is very slow and we'd rather focus it on the parts more relevant to the specific platform rather than on the generic code that's already well tested elsewhere. E.g. see the reduced bit usage here:

https://github.com/google/syzkaller/blob/7cc17001e974d9ff7d7ce042c4e2ad2e8ec0e55e/dashboard/config/linux/main.yml#L24C88-L24C95

Specifically in dashboard/config/linux/bits/subsystems.yml, I guess that the reasons were one of:

  • The intent similar to what I described above (we just did not use the -reduced condition instead, but should have done so).
  • We cannot fuzz the feature yet (no descriptions/not supported by the emulator).
  • The kernel was failing to build/boot with these enabled (though for these we usually leave an explicit comment).

If you believe that we'd better still fuzz some of them on the riscv syzbot instance, feel free to send a PR to switch them on or just let us know :)

@AlexGhiti AlexGhiti force-pushed the dev/alex/riscv_improvements branch from a15d19d to c636fa7 Compare January 23, 2025 13:34
@AlexGhiti
Copy link
Author

@a-nogikh Sorry to ping you, I'm not sure you were notified by my force-push. Or maybe you ignored it because of the ci/build failure? In that case, I would definitely need help to understand it :) Thanks

@a-nogikh
Copy link
Collaborator

Hi @AlexGhiti !
Feel free to write PTAL or explicitly re-request review in such cases :) Sometimes it's hard to distinguish the force pushes that we all while working on the PR and when the PR actually needs to be reviewed again.

Regarding the CI failures -- could you please rebase your changes on top of the latest master? We recently did fix some syzkaller bugs that could have led to the failures like the ones you got.

Alexandre Ghiti added 3 commits February 12, 2025 10:08
KASAN_INLINE was fixed back in early 2023 in the riscv kernel, see
merge commit 2667e3673f70 ("Merge patch series "RISC-V kasan rework").

It happens that the riscv configuration was already using KASAN_INLINE
so this is simply a cleanup.

Since this is my first commit in syzkaller, I also added myself and
Rivos in the AUTHORS/CONTRIBUTORS files.
Riscv is far from having a hw with a 5-level support, so let's focus on the
4-level.
kexec, memory hotplug/remove and THP features depend on architecture
specific code, so let's exercise this for riscv.
@AlexGhiti AlexGhiti force-pushed the dev/alex/riscv_improvements branch from c636fa7 to 67dad93 Compare February 13, 2025 08:59
@a-nogikh a-nogikh added this pull request to the merge queue Feb 13, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 13, 2025
@a-nogikh
Copy link
Collaborator

Filed #5775.
Let's try again.

@a-nogikh a-nogikh added this pull request to the merge queue Feb 13, 2025
Merged via the queue into google:master with commit 7320a65 Feb 13, 2025
17 checks passed
@AlexGhiti
Copy link
Author

Thanks @a-nogikh!

@a-nogikh
Copy link
Collaborator

Thanks for preparing the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants