[PATCH v4 2/4] rust: print: document safety of _printk FFI calls

From: Cian McGuire <[email protected]>

Two calls in this file had their SAFETY comments left as "TODO.": the
cast back to `fmt::Arguments` in `rust_fmt_argument`, and the call to
`bindings::_printk` in `call_printk`.

`rust_fmt_argument` is only reachable through the `%pA` format
specifier. Every in-tree Rust caller that uses `%pA` -- `call_printk`,
`device::printk`, `kunit::printk`, and `seq_file::call_printf` -- invokes
it synchronously with a pointer derived from its own live
`fmt::Arguments`, as one call in a `switch` dispatch in
`lib/vsprintf.c`'s pointer formatter that returns its result immediately;
there is no deferred use or storage of the pointer on the C side. So the
`fmt::Arguments` value stays alive on the caller's stack for the whole
time `rust_fmt_argument` is running with `ptr` pointing at it.

The `_printk` call itself is justified by this function's `# Safety`
section, which this patch also tightens: `format_string` must be one of
the non-continuation `format_strings` statics, not
`format_strings::CONT`. `call_printk` always supplies two extra
arguments -- `module_name` for `%s` and `args` for `%pA` -- which only
matches the `"%s: %pA"` shape shared by every non-continuation string;
`CONT`'s format string is `"%pA"` only, so passing it here would make
`_printk` consume `module_name`'s pointer for `%pA` instead, and
`rust_fmt_argument` would then reinterpret that `*const c_char` as a
`*const fmt::Arguments` and dereference it. No caller does this today --
`call_printk` is only reached from `print_macro!`'s non-continuation arm;
the continuation arm calls the separate, safe `call_printk_cont`, which
hardcodes `CONT` together with the single argument it expects -- but the
previous, broader contract permitted it. With `format_string` guaranteed
non-`CONT`, and `module_name` guaranteed null-terminated by both of its
possible origins (`kernel::__LOG_PREFIX` and the one generated by the
`module!` proc macro), the specifiers always match the supplied
arguments.

Suggested-by: Miguel Ojeda <[email protected]>
Link: https://github.com/Rust-for-Linux/linux/issues/351
Signed-off-by: Cian McGuire <[email protected]>
---
v4:
- Trim the `// SAFETY:` comment on the `_printk` call in `call_printk`
  down to what's specific to this call site (that `args` stays valid and
  is read back synchronously). The shape-matching reasoning it used to
  repeat is already covered by the `# Safety` section above it, which is
  the right place for it as a precondition rather than a per-call-site
  justification. (Gary Guo)
v3:
- Tightened `call_printk`'s `# Safety` contract to require a
  non-continuation format string (i.e. not `format_strings::CONT`),
  since the function always supplies both a `%s` and a `%pA` argument,
  which only matches the non-continuation strings' `"%s: %pA"` shape.
  Passing `CONT` under the old, broader contract would make `_printk`
  consume `module_name`'s pointer for `%pA` instead of `args`, which
  `rust_fmt_argument` would then wrongly reinterpret as a
  `*const fmt::Arguments` and dereference. Updated the inline SAFETY
  comment to match.
v2:
- Reworded the SAFETY comment on `rust_fmt_argument`'s cast to argue
  from the property that makes every `%pA` call site safe (synchronous
  invocation with a live `&fmt::Arguments`), instead of enumerating
  callers by name, so it doesn't go stale if a new caller is added.

 rust/kernel/print.rs | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index 6fd84389a858..3111f8f4c2e1 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -29,7 +29,10 @@
     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: This function is only reachable via the `%pA` format specifier; using `%pA` from
+    // Rust requires the caller to invoke it synchronously with a `ptr` derived from a live
+    // `&fmt::Arguments`, before that reference's scope can end. `ptr` is therefore valid and
+    // properly aligned for a `fmt::Arguments<'_>` for the duration of this call.
     let _ = w.write_fmt(unsafe { *ptr.cast::<fmt::Arguments<'_>>() });
     w.pos().cast()
 }
@@ -96,8 +99,11 @@ pub mod format_strings {
 ///
 /// # Safety
 ///
-/// The format string must be one of the ones in [`format_strings`], and
-/// the module name must be null-terminated.
+/// The format string must be one of the non-continuation strings in
+/// [`format_strings`] (i.e. not [`format_strings::CONT`]): this function
+/// always supplies both `module_name` and `args` as arguments, which only
+/// matches the `"%s: %pA"` shape used by the non-continuation strings, not
+/// `CONT`'s `"%pA"`-only shape. The module name must be null-terminated.
 ///
 /// [`_printk`]: srctree/include/linux/_printk.h
 #[doc(hidden)]
@@ -109,7 +115,9 @@ pub unsafe fn call_printk(
 ) {
     // `_printk` does not seem to fail in any path.
     #[cfg(CONFIG_PRINTK)]
-    // SAFETY: TODO.
+    // SAFETY: By this function's safety requirements, `format_string` and `module_name` match
+    // `_printk`'s expectations. `args` remains valid for the duration of this call and is only
+    // read back synchronously, through `%pA`, by `rust_fmt_argument`.
     unsafe {
         bindings::_printk(
             format_string.as_ptr(),
-- 
2.55.0