[PATCH] rust: add safety comment in rust_fmt_argument

[PATCH] rust: add safety comment in rust_fmt_argument

[Konstantin Andrikopoulos]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

The function rust_fmt_argument derefernces a c_void pointer, and thus it
needs an unsafe block. The safety comment for that block was missing.

Part of Rust-for-Linux#351.

Signed-off-by: Konstantin Andrikopoulos <kernel@mandragore.io>
---
 rust/kernel/print.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index a28077a7cb30..3545dd08fde0 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -24,7 +24,7 @@
     use fmt::Write;
     // SAFETY: The C contract guarantees that `buf` is valid if it's less than `end`.
     let mut w = unsafe { RawFormatter::from_ptrs(buf.cast(), end.cast()) };
-    // SAFETY: TODO.
+    // SAFETY: The Rust contract guarantees that `ptr` is a valid pointer to a `fmt::Arguments` struct.
     let _ = w.write_fmt(unsafe { *(ptr as *const fmt::Arguments<'_>) });
     w.pos().cast()
 }
-- 
2.47.0


-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wnUEARYIACcFAmc3wf4JEP6FRRdCoj7tFiEEZVtNSmVGsZsFReCA/oVFF0Ki
Pu0AAPvrAQCD8WeORM8+Nqsp4WQBmItlZz0S0fP5C9aUZpXjcUSmtgEAnMAV
CwlhJFanwZteGT8wM2Xodx2V8dWCmJxqiucOLwg=
=wJMv
-----END PGP SIGNATURE-----

Re: [PATCH] rust: add safety comment in rust_fmt_argument

[Miguel Ojeda]

On Fri, Nov 15, 2024 at 11:11 PM Konstantin Andrikopoulos
<kernel@mandragore.io> wrote:
>
> The function rust_fmt_argument derefernces a c_void pointer, and thus it
> needs an unsafe block. The safety comment for that block was missing.

Typo -- `scripts/checkpatch.pl` has a `--codespell` option that may help.

In addition, I would avoid the `c_void` mention, since it sounds like
the reason it is unsafe is because it is void in particular (and
anyway it is not true that we dereference a void pointer -- we cast it
first).

> Part of Rust-for-Linux#351.

This should typically be a Link tag, e.g.

Link: https://...

In addition, while it is not critical, the issue suggests adding a
"Suggested-by:" tag too (these issues are a good way to learn how
these tags work).

> +    // SAFETY: The Rust contract guarantees that `ptr` is a valid pointer to a `fmt::Arguments` struct.
>      let _ = w.write_fmt(unsafe { *(ptr as *const fmt::Arguments<'_>) });

Which Rust contract guarantees that? I guess perhaps you adapted the
message above that mentions "C contract" and/or you may be referring
to what is done in `call_printk`? I think we should be more explicit
here, and justify how the pointer ends up being a valid pointer to
`Arguments`. In other words, please try to explain how that happens.

Finally, please Cc the maintainers and reviewers in v2
(https://rust-for-linux.com/contributing#submitting-patches has a
couple lines you may want to use for that).

Thanks for the patch!

Cheers,
Miguel

Re: [PATCH] rust: add safety comment in rust_fmt_argument

[Konstantin Andrikopoulos]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Thanks for the comments! It seems I missed some simple stuff but I am taking notes for future reference!

On Saturday, November 16th, 2024 at 8:39 PM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> > + // SAFETY: The Rust contract guarantees that `ptr` is a valid pointer to a `fmt::Arguments` struct.
> > let _ = w.write_fmt(unsafe { *(ptr as *const fmt::Arguments<'_>) });
> 
> 
> Which Rust contract guarantees that? I guess perhaps you adapted the
> message above that mentions "C contract" and/or you may be referring
> to what is done in `call_printk`? I think we should be more explicit
> here, and justify how the pointer ends up being a valid pointer to
> `Arguments`. In other words, please try to explain how that happens.

Yes I was mainly trying to match the style of the existing comment since I wasn't sure how verbose I should get. And I agree that we can be more explicit. I am not sure _where_ we should be explicit though. I am new to the project so this might have been discussed already. It might be best to document the contract somewhere (possibly a module level docstring?). Then be explicit in each caller site on how the contract is upheld. And in rust_fmt_arguments we should simply be able to claim that %pA requires a pointer to a valid fmt::Arguments struct so it is ok to cast and deref.

Cheers,
Konstantin
-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wrsEARYKAG0Fgmc5/C0JkP6FRRdCoj7tRRQAAAAAABwAIHNhbHRAbm90YXRp
b25zLm9wZW5wZ3Bqcy5vcmeYIsGgZbOvfHtxEqDJwVNbBk5M3dEocTsU04Oh
RbcOvBYhBGVbTUplRrGbBUXggP6FRRdCoj7tAAAUCwD/duv59yJoOSn8cMAF
uM/ga3SV5jiacjFytdcLpAIT9FABAPRB3HQ2Doe/jQOrGg2/SJ4KWMPRmgRE
3XjnT/HIf8QM
=kpEK
-----END PGP SIGNATURE-----

Re: [PATCH] rust: add safety comment in rust_fmt_argument

[Miguel Ojeda]

On Sun, Nov 17, 2024 at 3:22 PM Konstantin Andrikopoulos
<kernel@mandragore.io> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> Thanks for the comments! It seems I missed some simple stuff but I am taking notes for future reference!

You're welcome!

By the way, applying your patch with `b4` keeps the PGP part -- I
would suggest `git send-email` or `b4` to send patches instead.

> Yes I was mainly trying to match the style of the existing comment since I wasn't sure how verbose I should get. And I agree that we can be more explicit. I am not sure _where_ we should be explicit though. I am new to the project so this might have been discussed already. It might be best to document the contract somewhere (possibly a module level docstring?). Then be explicit in each caller site on how the contract is upheld. And in rust_fmt_arguments we should simply be able to claim that %pA requires a pointer to a valid fmt::Arguments struct so it is ok to cast and deref.

Please feel free to remove the `#[expect(clippy::missing_safety_doc)]`
line to add safety preconditions (that `expect` is meant as a `TODO`
too). You may also want to clean up the `// SAFETY` comment in
`call_printk*` since they are related.

Thanks!

Cheers,
Miguel