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: <be2586e2-8d85-426e-975d-6f57da4ccf75@gmail.com>
Date: Mon, 15 Jul 2024 18:15:28 +0200
From: Michal Rostecki <vadorovsky@...il.com>
To: Björn Roy Baron <bjorn3_gh@...tonmail.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 15.07.24 17:56, Björn Roy Baron wrote:
> 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.
> 

OK, good point, I can indeed handle that in `optional_name!`.

>>
>> [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]