linux-kernel - Re: [PATCH v2 2/5] rust: maple

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: <DC9ADTLUFTUC.8OVFMY20FXLF@kernel.org>
Date: Fri, 22 Aug 2025 23:49:45 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com>
Cc: "Alice Ryhl" <aliceryhl@...gle.com>, "Andrew Morton"
 <akpm@...ux-foundation.org>, "Liam R. Howlett" <Liam.Howlett@...cle.com>,
 "Lorenzo Stoakes" <lorenzo.stoakes@...cle.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Andrew Ballance" <andrewjballance@...il.com>, "Boqun
 Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>, <linux-kernel@...r.kernel.org>,
 <maple-tree@...ts.infradead.org>, <rust-for-linux@...r.kernel.org>,
 <linux-mm@...ck.org>
Subject: Re: [PATCH v2 2/5] rust: maple_tree: add MapleTree

On Fri Aug 22, 2025 at 11:22 PM CEST, Miguel Ojeda wrote:
> As for being clearer, what we want to assert is that the cause is a
> given one, so `assert_eq!` seems like a natural choice (and it isn't a
> case like `is_none()` where there is an advantage). Plus it means it
> is able to show both sides if it fails. So it is not a clear win-win
> in my eyes.

As for

	assert_eq!(foo().unwrap_err().kind(), ErrorKind::NotFound);

vs.

	assert!(foo().is_err_and(|e| x.kind() == ErrorKind::NotFound));

the only thing I can think of is that the former fails differently for when
foo() is Ok() vs. the error kind does not match. I assume that's what you mean?

If so, I agree it's indeed a downside.

>> But especially people new to the kernel starting to write Rust drivers may not
>> even be aware of this fact. If they see some unwrap_err() calls in examples they
>> might not even thing about it a lot before starting to use them, e.g. because
>> they're used to it from userspace projects anyways.
>
> We still have the issue that they will see the `assert!` anyway and
> thus can easily think panics are OK. I understand what you are saying:
> you want to minimize those cases anyway.

Yeah, exactly. Another reason I'm less concernt about the assert!() is that I
think it's generally more more associated with test cases than unwrap(). But
this one might also be just my perception. :)

>> I think we should do this; I really think otherwise we gonna see a lot of them
>> once we get more drivers. It's just too convinient. :)
>
> A potential middle ground is to apply the lint in normal code, but not
> in examples.
>
> Or, even better, we can actually only allow it within `assert!`s,
> since it is a custom macro I wrote for KUnit and not the real one,
> i.e. enforcing what I suggested above [1].

I think this would be a great solution; thanks for pointing this out.

> Another one is a lint that just affects `unwrap()` and not
> `unwrap_err()` -- I think the Clippy one doesn't allow it now, but we
> could request it. It could be combined with the above so that only
> `unwrap_err()` is allowed within `assert!`s.
>
> We could also go the C way, warning in `checkpatch.pl` about it like
> it does `BUG_ON`.

I think your suggestion above is perfect, whereas I think this one is likely to
still slip through.

> What I like about the Clippy approach is that it can be locally `expect`ed.

Agreed!