linux-kernel - Re: [PATCH v4] Add methods for FromBytes trait.

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] [day] [month] [year] [list]

Message-ID: <D8J9SWEGNPVM.SVZXLT3LHVSA@proton.me>
Date: Tue, 18 Mar 2025 09:05:15 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Christian <christiansantoslima21@...il.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, ~lkcamp/patches@...ts.sr.ht, richard120310@...il.com
Subject: Re: [PATCH v4] Add methods for FromBytes trait.

On Tue Mar 18, 2025 at 12:14 AM CET, Christian wrote:
> Hi, Benno.
>
>> It usually is a good idea to include a changelog and a link to any prior
>> versions after this `---`. It won't be included in the final commit
>> message, but help reviewers and others keep track of this series.
>
> Yeah, my bad. I forgot.
>
>> I think this section should go before the `Safety` section.
>
> I followed this section:
> https://docs.kernel.org/rust/coding-guidelines.html#code-documentation,
> but no problem, I'll change.

Ah I see, I'll change that then.

>> Why is this trait becoming safe?
>
> I thought that if we change to a Result and get the Err case, it's not
> a problem to be safe.

A trait being `unsafe` means that the implementer needs to justify why
their implementation is correct. The fact that you change the return
type to `Result` doesn't change that the type must be transmutable from
sufficiently many bytes.

>> IMO it makes more sense for the return type to be `Option<&Self>`.
>
> I agree. I'll change.
>
>> This must also require that `Self: AsBytes`, since otherwise the user
>> could write padding bytes into the original slice.
>
> Did you mean `ToBytes`? Should I create another patch with an empty trait, e.g
> ```
> unsafe trait ToBytes {}
> ```
> or create the trait and its methods?

Nope, I mean `AsBytes`, it already exists in `rust/kernel/transmute.rs`.

>> Also the parameter name `mut_slice_of_bytes` is a bit long, how about
>> `bytes`?
>
> I liked it, I'll change to `bytes` and `bytes_mut`

I wouldn't put `_mut` in the parameter name, just name both of them
`bytes`.

>> What is this safety comment for?
>
> Idk if I should create another safety comment or just continue. In
> this case, I choose the first and submit the patch. So how should I
> proceed?

I don't understand, the safety comment that you added there doesn't make
any sense to me. I wouldn't have added it.

---
Cheers,
Benno