Document unsafe blocks

[00xc]

We should document all unsafe blocks in the codebase, explaining the rationale for why using the keyword is safe (i.e. // SAFETY: comments). This will allow us to enable the undocumented_unsafe_blocks clippy lint (e.g. cargo clippy -- -D clippy::undocumented_unsafe_blocks), and emit an error on new undocumented unsafe blocks.

[stefano-garzarella]

I started to look into that, but we have a looooot of unsafe blocks without any comment.
It will take time, but in the meantime we should enforce new PRs to carry SAFETY comments. Not sure if we should add some scripts in the CI for now, till we can enable undocumented_unsafe_blocks clippy lint.

[joergroedel]

I think we have to rely on reviews for making sure all new unsafe sections get a SAFETY comment.

[stefano-garzarella]

I think we have to rely on reviews for making sure all new unsafe sections get a SAFETY comment.

Yes, but we are human, and it can be missed, so I added a new pipeline for that, see #535

For example, we have recently added several with the work for user space (I will send another PR tomorrow to fix some of them, but I need help with some that I don't know how to document).

[vijaydhanraj]

Hi @stefano-garzarella, I am able to repro missing unsafe block comment with recently introduced userspace PRs using below changes. Please let me know if you want me to cook up a patch for fixing it.

diff --git a/Makefile b/Makefile
index 0916f1e1..26874350 100644
--- a/Makefile
+++ b/Makefile
@@ -175,10 +175,10 @@ bin/svsm-test.bin: bin/stage1-test
        objcopy -O binary $< $@
 
 clippy:
-       cargo clippy --workspace --all-features --exclude packit --exclude svsm-fuzz --exclude igvmbuilder --exclude igvmmeasure -- -D warnings
-       cargo clippy --workspace --all-features --exclude packit --exclude svsm-fuzz --exclude svsm --target=x86_64-unknown-linux-gnu -- -D warnings
-       RUSTFLAGS="--cfg fuzzing" cargo clippy --package svsm-fuzz --all-features --target=x86_64-unknown-linux-gnu -- -D warnings
-       cargo clippy --workspace --all-features --exclude packit --tests --target=x86_64-unknown-linux-gnu -- -D warnings
+       cargo clippy --workspace --all-features --exclude packit --exclude svsm-fuzz --exclude igvmbuilder --exclude igvmmeasure -- -D warnings -D clippy::undocumented_unsafe_blocks
+       cargo clippy --workspace --all-features --exclude packit --exclude svsm-fuzz --exclude svsm --target=x86_64-unknown-linux-gnu -- -D warnings -D clippy::undocumented_unsafe_blocks
+       RUSTFLAGS="--cfg fuzzing" cargo clippy --package svsm-fuzz --all-features --target=x86_64-unknown-linux-gnu -- -D warnings -D clippy::undocumented_unsafe_blocks
+       cargo clippy --workspace --all-features --exclude packit --tests --target=x86_64-unknown-linux-gnu -- -D warnings -D clippy::undocumented_unsafe_blocks
 
 clean:
        cargo clean  

[stefano-garzarella]

@vijaydhanraj I already have a patch here stefano-garzarella@5c2fb54

I'm going to post a PR with multiple SAFETY comments (not all, because they are too much for a single PR).
BTW if you think it's incomplete or incorrect, feel free to go head and post your version. Any help on this effort is more than welcome :-)

Just a note, instead of modifying the Makefile, you can just uncomment this line in Cargo.toml:

diff --git a/Cargo.toml b/Cargo.toml
index 1d3a7bf2..a1fe07a3 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -91,7 +91,7 @@ rest_pat_in_fully_bound_structs = "warn"
 string_to_string = "warn"
 suboptimal_flops = "warn"
 # TODO: fires many times, fix then enable.
-# undocumented_unsafe_blocks = "warn"
+undocumented_unsafe_blocks = "warn"
 unnecessary_box_returns = "warn"
 
 [workspace.metadata.scripts]

[vijaydhanraj]

Thanks @stefano-garzarella for the suggestion and the patch! The changes here stefano-garzarella@5c2fb54 look good to me.

[stefano-garzarella]

Just reopen this, not sure why GH decided to close it.