Unsafe blocks stats#18
Conversation
`head` seems not to handle pipe closure well, in fact especially when the test fails, we leave many `head` processes sleeping, even if the pipe is closed. Use `read`, and optimize the loop by avoiding opening the pipe every cycle, but assign it to fd 3. In this case `kill` may fail by finding the subprocess already out because the pipe can be already closed. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Split the IDT into an early-boot IDT (only large enough to contain exception vectors), which is allocated on the stack, and a global IDT (with full contents), which is allocated from persistent memory. This makes it much easier to make the IDT read-only after it has been populated. Signed-off-by: Jon Lange <jlange@microsoft.com>
Because `ImmutAfterInitCell` can be initialized by value as well as by reference, it is suitable for holding a global static reference. Therefore, `ImmutAfterInitRef` no longer adds any unique value, and the code can be cleaned up by removing it. Signed-off-by: Jon Lange <jlange@microsoft.com>
Reviewer's GuideRefactored Clippy execution into configurable Makefile targets, updated CI workflows to invoke make clippy with an undocumented unsafe blocks lint, and added a helper script to collect and compare unsafe‐block statistics. Flow diagram for unsafe blocks statistics collectionflowchart TD
Start([Start])
MakeClippy["make clippy UNSAFE_BLOCKS=1"]
ClippyOutput["Clippy output with undocumented unsafe blocks lint"]
UnsafeBlocksScript["scripts/unsafe_blocks_report.sh"]
Stats["Print stats: per module and total"]
End([End])
Start --> MakeClippy
MakeClippy --> ClippyOutput
ClippyOutput --> UnsafeBlocksScript
UnsafeBlocksScript --> Stats
Stats --> End
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe Makefile was updated to introduce configurable variables for Clippy options and arguments, enabling dynamic control over Clippy's behavior, particularly for undocumented unsafe blocks. Additionally, a new Bash script was added to analyze and report statistics on undocumented unsafe blocks detected by Clippy, supporting various output modes and command-line options. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script (unsafe_blocks_report.sh)
participant Makefile
participant Clippy
User->>Script (unsafe_blocks_report.sh): Run script with options
alt Input file provided
Script->>Script: Parse Clippy output from file
else No input file
Script->>Makefile: Invoke make clippy (with UNSAFE_BLOCKS=1)
Makefile->>Clippy: Run cargo clippy with custom args
Clippy-->>Makefile: Output diagnostics
Makefile-->>Script: Provide Clippy output
end
Script->>Script: Parse output, aggregate stats
Script->>User: Print report (per-module, total, or both)
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @stefano-garzarella - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `scripts/unsafe_blocks_report.sh:22` </location>
<code_context>
+ echo -e "Print statistics about undocumented unsafe blocks"
+ echo -e ""
+ echo -e "Generic options:"
+ echo -e " -f, --file FILENAME Use the specified file, istead of running clippy"
+ echo -e " -q, --quiet Print just stats without log messages"
+ echo -e " -h, --help Print this help"
</code_context>
<issue_to_address>
Typo in help message: 'istead' should be 'instead'.
Fixing the typo enhances clarity and professionalism.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
echo -e " -f, --file FILENAME Use the specified file, istead of running clippy"
=======
echo -e " -f, --file FILENAME Use the specified file, instead of running clippy"
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
Makefile (1)
192-197: Six nearly-identical invocations → hard to maintain
All fivecargo clippylines differ only by package/flags. Consider extracting into a pattern rule or phony target to avoid drift when flags change again.Example:
define CLIPPY_CALL cargo clippy $(CLIPPY_OPTIONS) $(1) -- $(CLIPPY_ARGS) $(2) endef clippy: $(call CLIPPY_CALL,--all-features --workspace --exclude svsm --exclude stage1 --exclude svsm-fuzz,) $(call CLIPPY_CALL,--all-features --package svsm-fuzz, ) ....github/workflows/rust.yml (1)
158-167: Hard-coded helper path may break when script moves
$GITHUB_WORKSPACE/scripts/unsafe_blocks_report.shis duplicated here; prefer${{ github.workspace }}/scripts/unsafe_blocks_report.shor simply./scripts/unsafe_blocks_report.sh(afterworking-directory:) to avoid path issues if the repository root changes.scripts/unsafe_blocks_report.sh (2)
22-30: Spelling / wording polish in help text
istead→instead- “stats are print” → “stats are printed”
- -f, --file FILENAME Use the specified file, istead of running clippy -By default all stats are print, if you want to select only some of them, + -f, --file FILENAME Use the specified file, instead of running clippy +By default all stats are printed. If you want to select only some of them,
32-63: Option parser: redundant wildcard pattern
ShellCheck flags SC2221/SC2222: the final*)pattern is shadowed because-*|--*)already matches any string beginning with-.
Swap the order to keep unknown-option detection while preserving the generic fallback:*) echo "Invalid parameter $1"; exit 1 ;; -*|--*) echo "Unknown option $1"; exit 1 ;;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/rust.yml(3 hunks)Makefile(2 hunks)scripts/unsafe_blocks_report.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/unsafe_blocks_report.sh
[warning] 53-53: This pattern always overrides a later one on line 53.
(SC2221)
[warning] 53-53: This pattern never matches because of a previous pattern on line 53.
(SC2222)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check unsafe blocks
- GitHub Check: Check
- GitHub Check: QEMU NOCC boot test
🔇 Additional comments (1)
.github/workflows/rust.yml (1)
115-118: Redirection hides clippy errors
make clippy … 2> clippy_warnings_pr.txtcaptures only stderr.
Ifcargo clippyemits a real error on stdout (it occasionally does for build-script failures), the CI step will succeed yet lose diagnostics.
Consider:make clippy … > /dev/null 2>clippy_warnings_pr.txtand append
|| { cat clippy_warnings_pr.txt; exit 1; }as you already do.
This keeps the current behaviour while ensuring genuine errors are not swallowed.
stefano-garzarella
force-pushed
the
unsafe-blocks-stats
branch
from
606dda2 to
dfac41a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
scripts/unsafe_blocks_report.sh (2)
3-4: Add-uand-o pipefailfor safer bash behaviour
set -ealone ignores failures that occur inside pipelines and allows the use of undefined variables.
Re-enable the hardening that was requested in previous reviews.-set -e +set -euo pipefail
72-85:grepexit-code still aborts the script when no matches are foundWith
set -e(set -euo pipefailafter the above fix) thegrepcommands in both helper functions make the whole script exit once the repo is fully documented (exit-code 1 == “no match”). Append|| trueso that the awk/pipe continues even with zero results.- grep -A 1 "${CLIPPY_SAFETY_MSG}" | grep "\-->" | awk '{print $2}' | \ + grep -A 1 "${CLIPPY_SAFETY_MSG}" | grep "\-->" || true | awk '{print $2}' | \Apply the same addition in
clippy_unsafe_blocks_total.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
-
.github/workflows/rust.yml(3 hunks) -
Makefile(2 hunks) -
scripts/unsafe_blocks_report.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/rust.yml
99-99: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
157-157: shellcheck reported issue in this script: SC2086:info:1:15: Double quote to prevent globbing and word splitting
(shellcheck)
157-157: shellcheck reported issue in this script: SC2086:info:2:17: Double quote to prevent globbing and word splitting
(shellcheck)
157-157: shellcheck reported issue in this script: SC2004:style:7:19:
(shellcheck)
157-157: shellcheck reported issue in this script: SC2004:style:7:34:
(shellcheck)
🪛 Shellcheck (0.10.0)
scripts/unsafe_blocks_report.sh
[warning] 53-53: This pattern always overrides a later one on line 53.
(SC2221)
[warning] 53-53: This pattern never matches because of a previous pattern on line 53.
(SC2222)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check
- GitHub Check: Check unsafe blocks
- GitHub Check: QEMU NOCC boot test
stefano-garzarella
force-pushed
the
unsafe-blocks-stats
branch
2 times, most recently
from
54081f4 to
b1ae262
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/unsafe_blocks_report.sh (2)
3-4: Addnounsetandpipefailfor robust error-handling
set -ealone misses failures inside pipelines and allows use of unset variables. Strengthen the safety switches:-set -e +set -euo pipefail
79-94:grepexits with 1 when no match → script abortsBecause
set -eis active, both helpers terminate the entire script when the repository is fully documented (zero matches). Append a harmless fallback so the pipeline still succeeds:- grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" | grep "\-->" | awk '{print $2}' | \ + grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" || true | grep "\-->" | awk '{print $2}' | \Apply the same
|| trueafter the firstgrepinclippy_unsafe_blocks_total.
🧹 Nitpick comments (3)
scripts/unsafe_blocks_report.sh (3)
96-104: Capture both stdout and stderr in one passCurrently only stderr is written to the temp file; any diagnostics that Clippy prints to stdout (rare but possible) are lost. Simpler and future-proof:
- make -C "${SCRIPT_DIR}/.." clippy CLIPPY_OPTIONS="--quiet --color=never" \ - UNSAFE_BLOCKS=1 2>"${TMP_FILE}" + make -C "${SCRIPT_DIR}/.." clippy CLIPPY_OPTIONS="--quiet --color=never" \ + UNSAFE_BLOCKS=1 &> "${TMP_FILE}"
23-38: Minor wording/grammar fixes in usage text‘stats are print’ → ‘stats are printed’; also capitalise headings consistently.
- echo -e "By default all stats are print, if you want to select only some of them," + echo -e "By default all stats are printed; to select only some of them,"
73-77: Preferprintfoverecho -efor predictable logging
echo -ehas portability quirks (e.g.,\ehandling);printfis POSIX-reliable.- echo -e "$@" + printf '%b\n' "$*"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Makefile(2 hunks)kernel/src/cpu/percpu.rs(8 hunks)kernel/src/hyperv/hv.rs(10 hunks)kernel/src/hyperv/mod.rs(3 hunks)kernel/src/mm/page_visibility.rs(3 hunks)kernel/src/sev/hv_doorbell.rs(2 hunks)scripts/unsafe_blocks_report.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- kernel/src/hyperv/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧬 Code Graph Analysis (3)
kernel/src/mm/page_visibility.rs (1)
kernel/src/mm/pagebox.rs (2)
deref(192-196)as_ref(224-226)
kernel/src/sev/hv_doorbell.rs (1)
kernel/src/mm/page_visibility.rs (1)
try_new_zeroed(111-136)
kernel/src/hyperv/hv.rs (4)
kernel/src/mm/page_visibility.rs (2)
try_new_zeroed(111-136)size_of(242-242)kernel/src/mm/address_space.rs (2)
virt_to_phys(67-74)virt_to_phys(100-103)kernel/src/cpu/percpu.rs (7)
new(85-89)new(175-180)new(192-199)new(267-279)new(418-447)new(1331-1338)new(1350-1354)kernel/src/cpu/mem.rs (1)
unsafe_copy_bytes(10-24)
🪛 Shellcheck (0.10.0)
scripts/unsafe_blocks_report.sh
[warning] 61-61: This pattern always overrides a later one on line 61.
(SC2221)
[warning] 61-61: This pattern never matches because of a previous pattern on line 61.
(SC2222)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: QEMU NOCC boot test
- GitHub Check: Check unsafe blocks
- GitHub Check: Check
🔇 Additional comments (14)
scripts/unsafe_blocks_report.sh (1)
40-71: Missing value check for-f/--fileargumentIf the last CLI parameter is
-fwithout a filename,$2is empty andshiftstill consumes it, leaving$CLIPPY_FILEunset and the script later fails. Validate before shifting:- -f | --file) - CLIPPY_FILE="$2" - shift + -f | --file) + [ -n "$2" ] || { echo "Error: --file requires an argument" >&2; exit 1; } + CLIPPY_FILE="$2" + shiftkernel/src/mm/page_visibility.rs (3)
174-180: LGTM! The pointer casting is well-documented.The cast from
*const NonNull<T>to*const *const Tis correctly justified by the transparency ofNonNull<T>over*mut Tand the layout compatibility between*mut Tand*const T.
219-228: Well-designed trait implementation with appropriate bounds.The
Derefimplementation correctly requiresT: FromBytes + Syncto ensure:
FromBytes: All bit patterns are valid, crucial for shared memory that may be modified by the hypervisorSync: Thread-safe access to the shared dataThe safety justification is sound.
230-234: Standard AsRef implementation.Correctly delegates to the
Derefimplementation following Rust idioms.kernel/src/sev/hv_doorbell.rs (2)
54-61: Excellent improvement to ownership semantics.Returning
SharedBox<HVDoorbell>instead of a leaked static reference:
- Preserves ownership for proper lifetime management
- Prevents memory leaks
- Aligns with the safer
SharedBoxabstractions used throughout the codebase
71-71: Good use of atomic types for concurrent safety.Replacing
UnsafeCell<[u8; 60]>with[AtomicU8; 60]provides proper synchronization for the reserved field that may be accessed concurrently by the hypervisor.kernel/src/cpu/percpu.rs (4)
403-403: Excellent type safety improvements.The field type changes significantly improve safety:
hypercall_pages: Now uses ownedHypercallPagetypes instead of raw pointershv_doorbell: UsesOnceCellfor initialization safety andSharedBoxfor proper shared memory managementAlso applies to: 406-406
602-606: Clean allocation using the new HypercallPage abstraction.The simplified allocation leverages the safe
HypercallPage::try_new()method, eliminating manual memory management.
608-614: Safe borrowing with RefMut.The updated method correctly uses
RefMut::mapto create aHypercallPagesGuardwith exclusive mutable access, eliminating unsafe code.
628-633: Proper use of the new ptr_ref API.The method correctly uses
SharedBox::ptr_ref()to obtain the pointer address and handles theNonecase appropriately.kernel/src/hyperv/hv.rs (4)
33-54: Excellent abstraction for hypercall page management.The
HypercallPagestruct properly encapsulates:
- Shared memory ownership via
SharedBox- Physical address tracking
- Safe allocation with
try_new()This is a significant safety improvement over raw pointer management.
56-69: Well-designed lifetime management.The two-lifetime design (
'aand'b) properly tracks:
'a: Lifetime of the borrow fromHypercallPagesGuard'b: Lifetime of the borrow intoPerCpuThis ensures compile-time safety for hypercall page access.
Also applies to: 125-136
175-207: Safe and ergonomic guard implementation.The refactored
HypercallPagesGuard:
- Uses
RefMutfor compile-time borrow checking- Safely splits the tuple with
RefMut::map_split- Maintains IRQ safety with
IrqGuardThis is a significant improvement in both safety and API design.
440-440: Good use of zerocopy traits for safe serialization.Adding
IntoBytesto hypercall input structures enables safe, zero-copy serialization into hypercall pages, eliminating manual unsafe memory operations.Also applies to: 483-483, 523-523
stefano-garzarella
force-pushed
the
unsafe-blocks-stats
branch
from
b1ae262 to
6c04547
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/unsafe_blocks_report.sh (2)
3-3: Still missingset -o pipefaildespite prior feedback
Withoutpipefail, failures of an early command in a pipeline are ignored, potentially masking missing-count regressions.
Add it right afterset -e(and consider-u).-set -e +set -euo pipefail
79-94: Pipelines will abort oncepipefailis added – add harmless fall-backWhen
pipefailis enabled,grepreturning 1 (zero matches) terminates the script.
Append|| trueafter eachgrepentry-point to keep “0 unsafe blocks” a valid condition.- grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" | grep "\-->" | awk '{print $2}' | \ + grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" || true | \ + grep "\-->" | awk '{print $2}' | \ ...Apply the same pattern in
clippy_unsafe_blocks_total.
🧹 Nitpick comments (2)
scripts/unsafe_blocks_report.sh (2)
81-88: Leading “/” in module prefixes hurts readabilityThe cumulative-path builder prints
/src,/src/arch, … which treats the repo root as a phantom directory.
Strip the first slash:- module=module "/" $i; + module=(module ? module "/" : "") $i;Produces
src,src/arch,src/arch/x86, matching usual module notation.
40-68:-*|--*pattern shadows subsequent cases (SC2221/SC2222)ShellCheck notes that the catch-all option pattern renders later pattern checks unreachable.
Moving it to the end (just before*) avoids accidental swallowing of-h|--helpetc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/unsafe_blocks_report.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.066Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
🪛 Shellcheck (0.10.0)
scripts/unsafe_blocks_report.sh
[warning] 61-61: This pattern always overrides a later one on line 61.
(SC2221)
[warning] 61-61: This pattern never matches because of a previous pattern on line 61.
(SC2222)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: QEMU NOCC boot test
- GitHub Check: Check unsafe blocks
- GitHub Check: nightly-check
- GitHub Check: Check
🔇 Additional comments (1)
scripts/unsafe_blocks_report.sh (1)
99-102: Clippy diagnostics may appear on stdout as wellCurrently only stderr is captured (
2>"${TMP_FILE}"). If future Rust/Cargo updates emit lint warnings on stdout, they’ll be missed and the script will report zero unsafe blocks.
Consider merging both streams:- make … 2>"${TMP_FILE}" + make … >"${TMP_FILE}" 2>&1
Update submodule to include a new commit documenting the crate's unsafe blocks. Signed-off-by: Carlos López <carlos.lopezr4096@gmail.com>
packit: update submodule
stefano-garzarella
force-pushed
the
unsafe-blocks-stats
branch
from
6c04547 to
832f5ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/unsafe_blocks_report.sh (2)
3-3: Add-o pipefail(and ideally-u) to ensure robust error-handling
set -ealone ignores failures that occur in the middle of a pipeline, so the script may silently under-count undocumentedunsafeblocks.
This exact gap was flagged in the previous review and remains unfixed.-set -e +set -euo pipefailIncluding
-uadditionally aborts on unset variables, avoiding surprises in CI.
79-94: Pipelines abort when no match is found – guard the initialgrepOnce
pipefailis enabled,grepwill exit 1 when no diagnostics are present and the whole function (and script) will terminate.
Wrap the firstgrepwith|| true(or usegrep -A … ||:) so zero-match situations are treated as “0 findings” instead of “CI failure”.- grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" | grep "\-->" | awk '{print $2}' | \ + grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" || true | grep "\-->" | awk '{print $2}' | \Do the same within
clippy_unsafe_blocks_total().Without this guard the script will fail precisely when the repository is fully documented—the opposite of what CI should reward.
🧹 Nitpick comments (1)
scripts/unsafe_blocks_report.sh (1)
61-64: Shell-pattern precedence hides the--*branchShellCheck (SC2221/SC2222) is right:
-*|--*is redundant because-*already matches anything beginning with-, so the--*half is dead code.- -*|--*) + -*)A tiny cleanup, but it removes confusion and the linter warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(2 hunks)packit(1 hunks)scripts/unsafe_blocks_report.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packit
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.066Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
scripts/unsafe_blocks_report.sh (1)
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.066Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
🪛 Shellcheck (0.10.0)
scripts/unsafe_blocks_report.sh
[warning] 61-61: This pattern always overrides a later one on line 61.
(SC2221)
[warning] 61-61: This pattern never matches because of a previous pattern on line 61.
(SC2222)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: QEMU NOCC boot test
- GitHub Check: Check unsafe blocks
- GitHub Check: nightly-check
- GitHub Check: Check
In commit 051e284 ("kernel/syscall: Implement two testing syscalls") we marked `current_task_terminated()` unsafe because it can panic, but this should not be related to memory safety. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
`slice::from_raw_parts` missed the safety comment. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
cpu: eliminate static IDT allocation
utils: remove `ImmutAfterInitRef`
…in-svsm-head scripts/test-in-svsm: avoid leaving `head` processes sleeping
…_unsafe kernel/task: make `current_task_terminated()` safe
…rnel-fs kernel/fs: add safety comment
stefano-garzarella
force-pushed
the
unsafe-blocks-stats
branch
from
832f5ed to
313e960
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
scripts/test-in-svsm.sh (1)
8-8: Addpipefailto avoid silent false-negatives in pipelines
set -edoes not abort the script when an early command in a pipeline fails.
Appending-o pipefail(and ideally-ufor unset vars) makes failures propagate and prevents the test from passing with hidden errors.-set -e +set -euo pipefail
♻️ Duplicate comments (2)
scripts/unsafe_blocks_report.sh (2)
1-4: Harden script withpipefail/nounsetWithout
pipefail, the grep/awk pipelines later in the script can fail silently, producing bogus “zero unsafe blocks” results.
nounsethelps catch typos in variable names.-#!/bin/bash -set -e +#!/bin/bash +set -euo pipefailNote: after enabling
pipefail, remember to|| truethegrepcommands below to keep “zero-matches” from aborting the run (see next comment).
80-94: Ensure zero-match grep doesn’t abort whenpipefailis activeWhen no undocumented unsafe blocks exist,
grepexits 1 which—underpipefail—will abort the function.
Mask the exit status so the pipeline still produces empty output that downstream commands handle gracefully.- grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" | grep "\-->" | awk '{print $2}' | \ + grep -A 1 "${CLIPPY_SAFETY_MSG}" "$1" || true | \ + grep "\-->" | awk '{print $2}' | \Apply the same
|| trueguard to theclippy_unsafe_blocks_totalfunction.
🧹 Nitpick comments (6)
scripts/test-in-svsm.sh (2)
15-16: Preserve whitespace and guard against dash-prefixed bytes
- Use
IFS=withreadso leading/trailing whitespace bytes (incl.\0x20) aren’t stripped.- Pass
--toprintfto avoid interpreting the byte as an option if it happens to start with “-”.- while read -r -n 1 -u 3 BYTE; do - TEST=$(printf '%s' "$BYTE" | xxd -p) + while IFS= read -r -n 1 -u 3 BYTE; do + TEST=$(printf '%s' -- "$BYTE" | xxd -p)
54-54: Preferwaitbeforekillfor cleaner shutdown
waitreaps the background job if it has already exited, eliminating the need for a redundantkill.
If the process is still alive,waitwill return when it ends naturally; only then fall back tokill -TERM(and eventuallykill -KILL) if required.# First try to reap gracefully wait "$TEST_IO_PID" 2>/dev/null || true # If still running, terminate kill "$TEST_IO_PID" 2>/dev/null || truescripts/unsafe_blocks_report.sh (2)
61-64: Redundant pattern in option parser (SC2221/SC2222)The case pattern
-*|--*)will always match before you ever reach the final*)pattern, rendering the latter effectively dead code.
Drop one of them or combine the error handling to silence the ShellCheck warning and keep the parser clear.
96-103: Clippy output: capture both streams or risk missing warningsClippy sometimes emits warnings on stdout rather than stderr depending on Rust/Cargo versions.
Redirecting only2>risks under-counting. Safer:- make -C "${SCRIPT_DIR}/.." clippy CLIPPY_OPTIONS="--quiet --color=never" \ - UNSAFE_BLOCKS=1 2>"${TMP_FILE}" + make -C "${SCRIPT_DIR}/.." clippy CLIPPY_OPTIONS="--quiet --color=never" \ + UNSAFE_BLOCKS=1 >"${TMP_FILE}" 2>&1kernel/src/cpu/idt/common.rs (2)
325-332: Consider enhancing the safety documentation.The current safety documentation should explicitly mention that this method intentionally leaks the page memory and that the returned IDT will have a
'staticlifetime. This is acceptable for IDT usage but should be clearly documented./// # Safety /// The caller is required to supply the virtual address of a freshly - /// allocated page. + /// allocated page. This method intentionally leaks the page memory + /// to create a 'static lifetime IDT, which is appropriate since the + /// IDT must remain valid for the entire system lifetime.
351-355: Consider handling empty slice edge case.While unlikely in practice, an empty entries slice would cause an underflow when calculating
limit(0 - 1 = usize::MAX). Consider adding a debug assertion or explicit check.pub fn base_limit(&self) -> (u64, u16) { + debug_assert!(!self.entries.is_empty(), "IDT must have at least one entry"); let base: *const IdtEntry = self.entries.as_ptr(); let limit = mem::size_of_val(self.entries) - 1; (base as u64, limit.try_into().unwrap()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Makefile(2 hunks)kernel/src/cpu/cpuid.rs(2 hunks)kernel/src/cpu/idt/common.rs(4 hunks)kernel/src/cpu/idt/mod.rs(1 hunks)kernel/src/cpu/idt/stage2.rs(1 hunks)kernel/src/cpu/idt/svsm.rs(5 hunks)kernel/src/cpu/smp.rs(2 hunks)kernel/src/cpu/vmsa.rs(2 hunks)kernel/src/fs/init.rs(1 hunks)kernel/src/stage2.rs(4 hunks)kernel/src/svsm.rs(3 hunks)kernel/src/syscall/class0.rs(1 hunks)kernel/src/task/schedule.rs(2 hunks)kernel/src/task/tasks.rs(1 hunks)kernel/src/utils/immut_after_init.rs(2 hunks)scripts/test-in-svsm.sh(3 hunks)scripts/unsafe_blocks_report.sh(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- kernel/src/syscall/class0.rs
- kernel/src/fs/init.rs
- kernel/src/cpu/vmsa.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.066Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
scripts/unsafe_blocks_report.sh (1)
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.066Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
🧬 Code Graph Analysis (5)
kernel/src/task/tasks.rs (1)
kernel/src/task/schedule.rs (1)
current_task_terminated(335-343)
kernel/src/cpu/smp.rs (1)
kernel/src/cpu/idt/svsm.rs (1)
load_static_idt(40-47)
kernel/src/cpu/idt/mod.rs (1)
kernel/src/cpu/idt/svsm.rs (1)
load_static_idt(40-47)
kernel/src/cpu/idt/stage2.rs (3)
kernel/src/cpu/control_regs.rs (1)
read_cr2(163-174)kernel/src/cpu/vc.rs (2)
stage2_handle_vc_exception(118-141)stage2_handle_vc_exception_no_ghcb(97-108)kernel/src/cpu/idt/svsm.rs (1)
early_idt_init(116-126)
kernel/src/cpu/idt/svsm.rs (6)
kernel/src/cpu/idt/common.rs (7)
user_mode(211-213)ist_entry(286-289)new(321-323)no_handler(301-303)new_from_page(328-332)entry(276-279)user_entry(281-284)kernel/src/task/tasks.rs (2)
is_task_fault(783-786)new(111-115)kernel/src/tdx/ve.rs (1)
handle_virtualization_exception(14-29)kernel/src/utils/immut_after_init.rs (1)
uninit(63-68)kernel/src/cpu/idt/stage2.rs (1)
early_idt_init(29-37)kernel/src/mm/pagebox.rs (1)
try_new_slice(142-150)
🪛 Shellcheck (0.10.0)
scripts/unsafe_blocks_report.sh
[warning] 61-61: This pattern always overrides a later one on line 61.
(SC2221)
[warning] 61-61: This pattern never matches because of a previous pattern on line 61.
(SC2222)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check unsafe blocks
- GitHub Check: Check
- GitHub Check: QEMU NOCC boot test
🔇 Additional comments (29)
kernel/src/task/tasks.rs (1)
843-843: LGTM! Unsafe block correctly removed.This change correctly reflects that
current_task_terminated()is now a safe function. The removal of the unsafe block aligns with the safety refactoring efforts described in the PR objectives.kernel/src/cpu/smp.rs (2)
7-7: Import update aligns with IDT refactoring.The change from importing
idttoload_static_idtcorrectly reflects the new static IDT initialization pattern.
84-84: Function call updated for static IDT loading.The switch from
idt().load()toload_static_idt()is consistent with the IDT refactoring that moves from dynamic access with locking to static initialization. This maintains the same functionality while improving safety and performance.kernel/src/task/schedule.rs (2)
335-335: Function signature correctly changed from unsafe to safe.The removal of the
unsafekeyword fromcurrent_task_terminated()is appropriate. The function body uses safe locking mechanisms and doesn't perform inherently unsafe operations like direct memory manipulation.
346-346: Unsafe block correctly removed from function call.Since
current_task_terminated()is now a safe function, the unsafe block is no longer needed. This change reduces unsafe code usage as intended by the PR objectives.kernel/src/cpu/idt/mod.rs (1)
11-12: Public exports updated for IDT refactoring.The changes correctly reflect the IDT refactoring:
- Removed dynamic access functions (
idt,idt_mut)- Added explicit IDT types and constants (
IdtEntry,EARLY_IDT_ENTRIES,IDT)- Added static IDT functionality (
load_static_idt,GLOBAL_IDT)This transition from dynamic to static IDT management improves safety and performance.
kernel/src/cpu/cpuid.rs (4)
9-9: Import updated for consolidated immutability pattern.The change from
ImmutAfterInitReftoImmutAfterInitCellaligns with the codebase consolidation on a single immutability-after-initialization pattern.
14-14: Static variable type updated consistently.The type change from
ImmutAfterInitRef<'_, SnpCpuidTable>toImmutAfterInitCell<&SnpCpuidTable>maintains the same semantics while using the consolidated API.
20-20: Method call updated for new API.The change from
.init_from_ref(table)to.init(table)correctly reflects theImmutAfterInitCellAPI.
42-42: Dereferencing updated for new type representation.The change from
&*CPUID_PAGEto*CPUID_PAGEcorrectly reflects howImmutAfterInitCell<&T>is dereferenced compared toImmutAfterInitRef.kernel/src/svsm.rs (2)
179-185: LGTM! Proper unsafe block with clear safety documentation.The early IDT initialization correctly creates a local array and passes it to
early_idt_initwith appropriate safety comments explaining the lifetime requirements.
260-260: LGTM! Appropriate error handling for critical initialization.The change correctly handles the
Resultreturn type fromidt_init(). Usingexpect()is appropriate here since IDT initialization failure is unrecoverable during boot.kernel/src/cpu/idt/stage2.rs (2)
13-24: LGTM! Well-documented unsafe function with clear safety contract.The function correctly documents its safety requirements, and the internal unsafe block for
idt.load()properly delegates the safety guarantee to the caller.
26-37: LGTM! Consistent safety documentation pattern.The function follows the same pattern as
early_idt_init_no_ghcbwith proper safety documentation and consistent unsafe block handling.kernel/src/stage2.rs (3)
101-113: LGTM! Proper safety documentation and lifetime management.The function correctly documents the safety requirements for the IDT lifetime, and the unsafe block properly delegates this guarantee when calling
early_idt_init_no_ghcb.
166-169: LGTM! Consistent unsafe block usage.The second IDT initialization call maintains the same safety pattern and documentation.
403-412: LGTM! Correct lifetime management for early IDT.The early IDT is stack-allocated in
stage2_main(which never returns), ensuring it remains valid throughout stage2 execution. The safety comment correctly explains this guarantee.kernel/src/utils/immut_after_init.rs (2)
101-103: LGTM! Clear safety documentation added.The safety comment correctly explains why the
assume_init_ref()call is safe after the initialization check.
174-177: LGTM! Improved formatting of safety comments.The reformatted safety comments for the
SendandSyncimplementations are more concise and clear.kernel/src/cpu/idt/svsm.rs (4)
38-48: LGTM! Well-structured global IDT management.The
GLOBAL_IDTstatic andload_static_idtfunction provide a clean interface for managing the global IDT with proper initialization checks and safety documentation.
83-111: LGTM! Clean refactoring to explicit IDT parameters.The functions now take explicit
&mut IDTparameters instead of accessing global mutable state, which improves safety and makes dependencies explicit.
113-126: LGTM! Proper unsafe function with clear safety contract.The
early_idt_initfunction correctly documents its safety requirements and properly delegates the lifetime guarantee to the caller.
128-169: LGTM! Well-structured IDT initialization with proper memory management.The function correctly:
- Allocates page-aligned memory for the IDT using
PageBox- Initializes all vectors appropriately
- Handles errors with
Result<(), SvsmError>- Documents safety requirements for the IDT loading
- Properly initializes the global IDT
The use of
PageBox::leakto get a static lifetime is appropriate since the IDT needs to persist for the lifetime of the kernel.kernel/src/cpu/idt/common.rs (6)
16-16: Import addition looks good.The
PageBoximport is appropriately placed and necessary for the newnew_from_pagemethod.
306-306: Appropriate constant for early IDT initialization.The value of 32 correctly covers all x86 CPU exception vectors (0-31), which is sufficient for early boot stages.
309-313: Field rename improves accuracy.Renaming from
sizetolimitcorrectly reflects x86 IDT descriptor terminology where this field represents the byte limit (size - 1) of the IDT.
315-323: Excellent refactoring for flexibility and safety.The transition to a slice-based design with explicit lifetime management provides better flexibility while maintaining memory safety through Rust's lifetime system.
357-373: Good refactoring of the load method.Using the
base_limit()method eliminates code duplication and improves maintainability.
376-388: Field name updated consistently.The change from
sizetolimitis consistent with theIdtDescstruct field rename.
Because `RunQueue` can be accessed by multiple threads via a reader/writer lock, it must be `Sync`. This is not possible as long as it embeds a `OnceCell`, used today for the idle task. The idle task can be set with a writer lock and therefore does not require a `OnceCell`. Signed-off-by: Jon Lange <jlange@microsoft.com>
Switching to `AtomicLink` makes the `VMM` structure `Sync`. Signed-off-by: Jon Lange <jlange@microsoft.com>
Use of reader/writer locks requires that the underlying objects be `Send` + `Sync`, so these trait bounds should be included in the `Mapping` object. This also eliminates the need to manually implement `Safe` and `Sync` for `Mapping`. Signed-off-by: Jon Lange <jlange@microsoft.com>
When acquiring a reader/writer lock for read access, the data protected by the lock may be accessible to multiple threads. Therefore, such operations require the inner type to be `Sync`. Other operations on locks guarantee exclusive access to the data, and therefore those operations only require `Send`. Signed-off-by: Jon Lange <jlange@microsoft.com>
Document all uses of unsafe in the fuzzing harnesses. Signed-off-by: Carlos López <carlos.lopezr4096@gmail.com>
fuzz: add safety comments
Document and clean up use of `unsafe` blocks in SEV utilties and calling functions. Signed-off-by: Jon Lange <jlange@microsoft.com>
Document additional `unsafe` blocks in stage2. Signed-off-by: Jon Lange <jlange@microsoft.com>
stage2: add safety comments
Page validation/invalidation operations affect memory safety and must be marked as `unsafe`. Signed-off-by: Jon Lange <jlange@microsoft.com>
sev: add safety comments
kernel: clean up `Send` and `Sync` bounds on locks
platform: mark page validation as `unsafe`
`make clippy UNSAFE_BLOCKS=1` will enable `clippy::undocumented_unsafe_blocks` warnings. This can help the effort to reduce them. Add also `-W warnings` to disable `-D warnings` enabled by default, otherwise `clippy` will exit after finding the first module that contains them. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
CLIPPY_OPTIONS can be used to pass options to clippy (e.g. --quiet --color=never) when `make clippy` is called by scripts/CI. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
The new script can be used to print statistics about undocumented unsafe blocks in our code base. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
stefano-garzarella
force-pushed
the
unsafe-blocks-stats
branch
from
313e960 to
b9f148d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/unsafe_blocks_report.sh (3)
3-4: Addpipefailto prevent silent pipeline failuresThe script needs
-o pipefailto catch failures in pipeline commands.
30-30: Fix typo: 'istead' should be 'instead'
79-95: Handle grep exit code when no matches foundWhen no unsafe blocks exist, grep returns exit code 1, causing the script to fail due to
set -e.
🧹 Nitpick comments (2)
kernel/src/mm/vm/mapping/api.rs (1)
181-182: Manual Send/Sync implementations need justificationWhile the manual
unsafe impl Send/Sync for VMMis likely correct given the internal synchronization viaRWLockand atomic operations, consider adding a safety comment explaining why these implementations are sound.+// SAFETY: VMM is Send/Sync because: +// - `link` uses AtomicLink which is thread-safe +// - `range` is only modified under external locking (as noted in the struct comment) +// - `mapping` is protected by Arc<RWLock<...>> with Send+Sync bounds unsafe impl Send for VMM {} unsafe impl Sync for VMM {}kernel/src/sev/utils.rs (1)
317-318: Minor typo in safety commentThere's a typo in the safety comment.
- // as a guest VMSA< which implies that the memory no longer has safety + // as a guest VMSA, which implies that the memory no longer has safety
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
Makefile(2 hunks)fuzz/fuzz_targets/acpi.rs(1 hunks)fuzz/fuzz_targets/alloc.rs(4 hunks)fuzz/fuzz_targets/fs.rs(1 hunks)fuzz/fuzz_targets/page_alloc.rs(4 hunks)kernel/src/cpu/percpu.rs(1 hunks)kernel/src/igvm_params.rs(1 hunks)kernel/src/locking/rwlock.rs(5 hunks)kernel/src/locking/spinlock.rs(1 hunks)kernel/src/mm/page_visibility.rs(2 hunks)kernel/src/mm/vm/mapping/api.rs(6 hunks)kernel/src/platform/mod.rs(1 hunks)kernel/src/platform/native.rs(1 hunks)kernel/src/platform/snp.rs(2 hunks)kernel/src/platform/snp_fw.rs(2 hunks)kernel/src/platform/tdp.rs(2 hunks)kernel/src/protocols/core.rs(6 hunks)kernel/src/sev/ghcb.rs(2 hunks)kernel/src/sev/utils.rs(10 hunks)kernel/src/sev/vmsa.rs(2 hunks)kernel/src/stage2.rs(18 hunks)kernel/src/task/schedule.rs(6 hunks)scripts/unsafe_blocks_report.sh(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- fuzz/fuzz_targets/fs.rs
- fuzz/fuzz_targets/acpi.rs
- kernel/src/mm/page_visibility.rs
- kernel/src/sev/vmsa.rs
- fuzz/fuzz_targets/page_alloc.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- kernel/src/task/schedule.rs
- kernel/src/stage2.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.116Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
kernel/src/platform/snp_fw.rs (1)
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.116Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
scripts/unsafe_blocks_report.sh (1)
Learnt from: stefano-garzarella
PR: stefano-garzarella/svsm#18
File: Makefile:12-16
Timestamp: 2025-06-25T16:46:41.116Z
Learning: In the Makefile, when UNSAFE_BLOCKS is set, CLIPPY_ARGS is intentionally overridden (not appended) to replace `-D warnings` with `-W clippy::undocumented_unsafe_blocks`. This allows clippy to continue running through the entire codebase to collect all instances of undocumented unsafe blocks, rather than exiting on the first warning/error from any lint.
🧬 Code Graph Analysis (8)
kernel/src/sev/ghcb.rs (4)
kernel/src/sev/utils.rs (1)
pvalidate(126-167)kernel/src/hyperv/hv.rs (1)
vaddr(51-53)kernel/src/mm/pagebox.rs (1)
vaddr(119-121)kernel/src/sev/msr_protocol.rs (1)
validate_page_msr(204-207)
kernel/src/platform/mod.rs (3)
kernel/src/platform/native.rs (1)
validate_virtual_page_range(160-178)kernel/src/platform/snp.rs (1)
validate_virtual_page_range(286-294)kernel/src/platform/tdp.rs (1)
validate_virtual_page_range(193-207)
kernel/src/platform/snp_fw.rs (2)
kernel/src/sev/utils.rs (2)
pvalidate(126-167)rmp_adjust(249-290)kernel/src/sev/vmsa.rs (1)
vaddr(54-57)
kernel/src/platform/native.rs (3)
kernel/src/platform/mod.rs (1)
validate_virtual_page_range(179-183)kernel/src/platform/snp.rs (1)
validate_virtual_page_range(286-294)kernel/src/platform/tdp.rs (1)
validate_virtual_page_range(193-207)
fuzz/fuzz_targets/alloc.rs (1)
kernel/src/mm/alloc.rs (2)
alloc(1656-1669)dealloc(1672-1693)
kernel/src/platform/snp.rs (4)
kernel/src/sev/utils.rs (2)
pvalidate_range(75-113)from(27-29)kernel/src/platform/native.rs (2)
new(47-56)validate_virtual_page_range(160-178)kernel/src/platform/mod.rs (2)
new(241-253)validate_virtual_page_range(179-183)kernel/src/platform/tdp.rs (3)
new(61-63)new(286-288)validate_virtual_page_range(193-207)
kernel/src/protocols/core.rs (1)
kernel/src/sev/utils.rs (4)
rmp_clear_guest_vmsa(331-335)rmp_set_guest_vmsa(314-326)pvalidate(126-167)rmp_grant_guest_access(306-309)
kernel/src/locking/rwlock.rs (2)
kernel/src/cpu/irq_state.rs (1)
irq_state(396-410)kernel/src/locking/common.rs (4)
acquire_lock(28-28)acquire_lock(38-40)acquire_lock(54-59)acquire_lock(73-78)
🪛 Shellcheck (0.10.0)
scripts/unsafe_blocks_report.sh
[warning] 61-61: This pattern always overrides a later one on line 61.
(SC2221)
[warning] 61-61: This pattern never matches because of a previous pattern on line 61.
(SC2222)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check unsafe blocks
- GitHub Check: Check
- GitHub Check: nightly-check
- GitHub Check: QEMU NOCC boot test
🔇 Additional comments (22)
kernel/src/locking/rwlock.rs (3)
103-106: Good safety documentation for unconditional Send/Sync implementations.The removal of trait bounds from the unsafe impl blocks and addition of safety comments correctly reflects that well-formed locks are inherently Send and Sync, regardless of the protected type's bounds. This aligns with the pattern used in std::sync::Mutex and similar primitives.
146-256: Excellent refactoring of trait bounds for lock operations.The split into separate impl blocks with
T: Sendfor write operations andT: Send + Syncfor read operations correctly models the safety requirements:
- Write locks only need
Sendsince they provide exclusive access- Read locks need
Send + Syncsince they allow shared access across threadsThe implementation correctly manages the reader/writer state transitions.
258-291: Read lock implementation correctly enforces Sync requirement.The additional
Syncbound for read operations is necessary and well-documented. The implementation properly manages concurrent reader access while ensuring writers are excluded.kernel/src/locking/spinlock.rs (1)
116-123: Consistent safety documentation for SpinLock.The changes mirror those in
rwlock.rs, maintaining consistency across locking primitives. The unconditionalSend/Syncimplementations with safety comments and theT: Sendbound on the impl block are correct.kernel/src/cpu/percpu.rs (1)
882-882: Correct lock type for mutable runqueue access.The change from
lock_read()tolock_write()is necessary sinceset_idle_taskrequires mutable access to the runqueue. This aligns with the mentioned changes toRunQueuewhere the idle task setter now requires&mut self.kernel/src/igvm_params.rs (1)
215-220: Proper unsafe block with clear safety justification.The explicit unsafe block with safety comment correctly documents why the operation is safe - the virtual address region was created specifically to map the physical address range on lines 198-200. This aligns with the broader effort to make unsafe operations explicit throughout the codebase.
kernel/src/sev/ghcb.rs (1)
148-152: Improved placement of safety documentation.Moving the comments inside the unsafe blocks better associates the safety explanations with the specific operations they describe. This improves code clarity and maintainability.
Also applies to: 189-196
kernel/src/platform/mod.rs (1)
176-183: LGTM! Trait method correctly made unsafe with clear safety contract.The addition of the
unsafemodifier and safety documentation tovalidate_virtual_page_rangein the trait definition is appropriate. This enforces that all implementors and callers must explicitly handle the safety contract, which is consistent with the memory validation operations performed by the platform implementations.kernel/src/platform/snp_fw.rs (2)
189-202: Properly documented unsafe blocks for memory validation operations.The addition of explicit
unsafeblocks aroundpvalidateandrmp_adjustcalls is correct. The safety comments appropriately document that the virtual address mapping corresponds to the guest physical address range, which justifies the safety of these low-level memory validation operations.
383-390: Correctly wrapped unsafe operation with appropriate safety documentation.The
unsafeblock around thermp_adjustcall is properly justified with the safety comment noting that the address is known to be a guest page. This aligns with the explicit safety contract required by thermp_adjustfunction.kernel/src/platform/native.rs (1)
157-160: Implementation correctly matches unsafe trait signature.The addition of the
unsafemodifier and safety documentation to thevalidate_virtual_page_rangeimplementation is appropriate and consistent with the trait definition. The safety contract documentation aligns with other platform implementations while maintaining the native platform's debugging behavior.kernel/src/platform/tdp.rs (1)
190-204: Well-executed safety contract implementation with improved documentation.The changes correctly implement the unsafe trait method with appropriate safety documentation. The updated safety comment in the
Validatearm appropriately shifts responsibility to the caller rather than referencing "pending safety verification," which improves clarity about the safety contract.kernel/src/platform/snp.rs (2)
66-70: Appropriately documented unsafe block for physical-to-virtual mapping operation.The addition of the
unsafeblock aroundpvalidate_rangeis correct, with the safety comment properly justifying that the mapping correctly represents the physical address range. This ensures the low-level validation operation is performed safely.
283-294: Consistent implementation of unsafe trait method with proper safety delegation.The changes correctly implement the unsafe
validate_virtual_page_rangemethod with appropriate safety documentation. The safety comment properly delegates responsibility to the caller, and the unsafe block aroundpvalidate_rangemaintains the explicit safety contract throughout the call chain.kernel/src/protocols/core.rs (2)
61-161: Well-documented unsafe blocks with clear safety contractsThe unsafe function declaration and all unsafe block usages have appropriate safety comments explaining the caller's obligations. The changes correctly enforce Rust's safety model for these low-level operations.
188-189: Consistent safety documentation throughout the fileAll unsafe blocks for RMP and pvalidate operations maintain consistent safety documentation patterns, making the code's safety requirements clear and auditable.
Also applies to: 307-312, 355-357
fuzz/fuzz_targets/alloc.rs (2)
53-122: Excellent safety documentation for allocator methodsThe safety contracts are clearly documented for each method, and the transition of
allocfrom unsafe to safe with internal unsafe blocks is appropriate since it properly handles the safety invariants internally.
158-197: Thorough safety comments at all usage sitesEach unsafe call site documents why the operation is safe, maintaining clear provenance of pointers from the allocator.
kernel/src/mm/vm/mapping/api.rs (1)
14-14: Good concurrency improvements with AtomicLink and explicit trait boundsThe migration to
AtomicLinkand explicitSend + Syncbounds on the trait object improve thread safety. The changes ensure the intrusive RBTree operations are thread-safe.Also applies to: 141-161, 167-167, 184-184, 207-207, 238-244
kernel/src/sev/utils.rs (3)
55-113: Comprehensive safety documentation for pvalidate operationsThe safety contracts clearly specify that callers must ensure memory safety is not violated by the validation operations. The internal unsafe blocks have appropriate safety comments.
Also applies to: 123-167
170-194: Enhanced safety documentation for raw_vmmcallThe expanded safety comment properly explains the memory safety implications of VMMCALL handlers potentially treating registers as pointers.
245-335: Well-structured safety contracts for RMP operationsAll RMP-related functions have clear safety documentation explaining when memory safety could be affected (e.g., exposing pages to lower VMPLs or converting to VMSA pages).
| usage | ||
| exit | ||
| ;; | ||
| -*|--*) |
There was a problem hiding this comment.
Fix unreachable pattern matching case
The pattern -*|--* will always match on -*, making --* unreachable. This should be two separate cases.
- -*|--*)
+ -*)
+ echo "Unknown option $1"
+ exit 1
+ ;;
+ --*)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -*|--*) | |
| - -*|--*) | |
| + -*) | |
| + echo "Unknown option $1" | |
| + exit 1 | |
| + ;; | |
| + --*) |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 61-61: This pattern always overrides a later one on line 61.
(SC2221)
[warning] 61-61: This pattern never matches because of a previous pattern on line 61.
(SC2222)
🤖 Prompt for AI Agents
In scripts/unsafe_blocks_report.sh at line 61, the pattern matching case '*|--*'
is incorrect because the first pattern '-*' already matches all cases starting
with '-', making the '--*' pattern unreachable. To fix this, split the combined
pattern into two separate cases: one for '-*' and another for '--*', ensuring
both patterns are handled distinctly and reachable.
5 participants
Summary by Sourcery
Introduce tooling to track, report, and enforce documentation of unsafe code blocks by parameterizing clippy invocations and integrating a new reporting script into CI
New Features:
unsafe_blocks_report.shto compute statistics for undocumented unsafe code blocksEnhancements:
CLIPPY_OPTIONSandUNSAFE_BLOCKSflags to toggle the undocumented unsafe-blocks lintCI:
make clippy UNSAFE_BLOCKS=1and use the new script to compare undocumented unsafe block counts between PR and base branchSummary by CodeRabbit