netdev - Re: [PATCH] rust: str: Use `core::CStr`, remove the custom `CStr` implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <T4cW5BFYytkMlTR5e2C2FfFJ5Z8P5XPw5dEsTQ2V-hoAo5yZkeYLSU3GvVCTH1Ga3f-mbPvEKZxOEWT7E1-xWu4EDE6-jCoQj3If-qCKCHA=@protonmail.com>
Date: Mon, 15 Jul 2024 15:56:39 +0000
From: Björn Roy Baron <bjorn3_gh@...tonmail.com>
To: Michal Rostecki <vadorovsky@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Brendan Higgins <brendan.higgins@...ux.dev>, David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>, FUJITA Tomonori <fujita.tomonori@...il.com>, Trevor Gross <tmgross@...ch.edu>, Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, Martin Rodriguez Reboredo <yakoyoku@...il.com>, Finn Behrens <me@...enk.dev>, Manmohan Shukla <manmshuk@...il.com>, Valentin Obst <kernel@...entinobst.de>, Laine Taffin Altman <alexanderaltman@...com>, Danilo Krummrich <dakr@...hat.com>, Yutaro Ohno <yutaro.ono.418@...il.com>, Tiago Lam <tiagolam@...il.com>, Charalampos Mitrodimas <charmitro@...teo.net>,
	Ben Gooding <ben.gooding.dev@...il.com>, Roland Xu <mu001999@...look.com>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, netdev@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] rust: str: Use `core::CStr`, remove the custom `CStr` implementation

On Monday, July 15th, 2024 at 17:46, Michal Rostecki <vadorovsky@...il.com> wrote:

> On 14.07.24 19:01, Björn Roy Baron wrote:
> > On Sunday, July 14th, 2024 at 18:02, Michal Rostecki <vadorovsky@...il.com> wrote:
> >
> >> `CStr` became a part of `core` library in Rust 1.75, therefore there is
> >> no need to keep the custom implementation.
> >>
> >> `core::CStr` behaves generally the same as the removed implementation,
> >> with the following differences:
> >>
> >> - It does not implement `Display` (but implements `Debug`).
> >> - It does not provide `from_bytes_with_nul_unchecked_mut` method.
> >>    - It was used only in `DerefMut` implementation for `CString`. This
> >>      change replaces it with a manual cast to `&mut CStr`.
> >>    - Otherwise, having such a method is not really desirable. `CStr` is
> >>      a reference type
> >>      or `str` are usually not supposed to be modified.
> >> - It has `as_ptr()` method instead of `as_char_ptr()`, which also returns
> >>    `*const c_char`.
> >>
> >> Rust also introduces C literals (`c""`), so the `c_str` macro is removed
> >> here as well.
> >>
> >> Signed-off-by: Michal Rostecki <vadorovsky@...il.com>
> >> ---
> >>   rust/kernel/error.rs        |   7 +-
> >>   rust/kernel/init.rs         |   8 +-
> >>   rust/kernel/kunit.rs        |  16 +-
> >>   rust/kernel/net/phy.rs      |   2 +-
> >>   rust/kernel/prelude.rs      |   4 +-
> >>   rust/kernel/str.rs          | 490 +-----------------------------------
> >>   rust/kernel/sync.rs         |  13 +-
> >>   rust/kernel/sync/condvar.rs |   5 +-
> >>   rust/kernel/sync/lock.rs    |   6 +-
> >>   rust/kernel/workqueue.rs    |  10 +-
> >>   scripts/rustdoc_test_gen.rs |   4 +-
> >>   11 files changed, 57 insertions(+), 508 deletions(-)
> >>
> >
> > [snip]
> >
> >> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> >> index 68605b633e73..af0017e56c0e 100644
> >> --- a/rust/kernel/init.rs
> >> +++ b/rust/kernel/init.rs
> >> @@ -46,7 +46,7 @@
> >>   //! }
> >>   //!
> >>   //! let foo = pin_init!(Foo {
> >> -//!     a <- new_mutex!(42, "Foo::a"),
> >> +//!     a <- new_mutex!(42, c"Foo::a"),
> >
> > That we need a CStr here seems a bit of an internal implementation detail. Maybe
> > keep accepting a regular string literal and converting it to a CStr internally?
> > If others think what you have here is fine, I don't it mind all that much though.
> >
> 
> The names passed to `new_mutex`, `new_condvar`, `new_spinlock` etc. are
> immediately passed in the FFI calls (`__mutex_init`,
> `__init_waitqueue_head`, `__spin_lock_init`) [0][1][2]. In fact, I don't
> see any internal usage, where using Rust &str would be beneficial. Am I
> missing something?
> 
> Converting a &str to &CStr inside `Mutex::new` or `CondVar::new` would
> require allocating a new buffer, larger by 1, to include the nul byte.
> Doing that for every new mutex or condvar seems a bit wasteful to me.

The names passed to `new_mutex!` and such are literals known at
compile time. This means we can keep adding the nul terminator at
compile time without allocating any extra buffer. Basically just
adapting the current implementation of `optional_name!` to produce an
`core::ffi::&CStr` rather than a `kernel::str::CStr` from a regular
string literal is enough to avoid having to explicitly use C string
literals in those macro invocations. This way users don't need to
know that internally an `&CStr` is used.

> 
> [0]
> https://github.com/Rust-for-Linux/linux/blob/b1263411112305acf2af728728591465becb45b0/rust/kernel/sync/lock/mutex.rs#L104
> [1]
> https://github.com/Rust-for-Linux/linux/blob/b1263411112305acf2af728728591465becb45b0/rust/kernel/sync/condvar.rs#L111
> [2]
> https://github.com/Rust-for-Linux/linux/blob/b1263411112305acf2af728728591465becb45b0/rust/kernel/sync/lock/spinlock.rs#L103

[snip]