linux-kernel - Re: [PATCH] rust: io: use const generics for read/write offsets

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <752F0825-6F2E-4AC0-BEBD-2E285A521A22@nvidia.com>
Date: Fri, 19 Sep 2025 07:59:21 +0000
From: Joel Fernandes <joelagnelf@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
CC: Alice Ryhl <aliceryhl@...gle.com>, Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
	Simona Vetter <simona@...ll.ch>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Miguel
 Ojeda <ojeda@...nel.org>, 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>, Bjorn
 Helgaas <bhelgaas@...gle.com>, Krzysztof Wilczyński
	<kwilczynski@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "rust-for-linux@...r.kernel.org"
	<rust-for-linux@...r.kernel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>
Subject: Re: [PATCH] rust: io: use const generics for read/write offsets

Hello, Danilo,

> On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <dakr@...nel.org> wrote:
> 
> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
>>> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
>>> Using build_assert! to assert that offsets are in bounds is really
>>> fragile and likely to result in spurious and hard-to-debug build
>>> failures. Therefore, build_assert! should be avoided for this case.
>>> Thus, update the code to perform the check in const evaluation instead.
>> 
>> I really don't think this patch is a good idea (and nobody I spoke to thinks
>> so). Not only does it mess up the user's caller syntax completely, it is also
> 
> I appreacite you raising the concern,
> but I rather have other people speak up
> themselves.

I did not mean to speak for others, sorry it came across like that (and that is certainly not what I normally do). But I discussed the patch in person since we are at a conference and discussing it in person, and I did not get a lot of consensus on this. That is what I was trying to say. If it was a brilliant or great idea, I would have hoped for at least one person to tell me that this is exactly how we should do it.

> 
>> super confusing to pass both a generic and a function argument separately.
> 
> Why? We assert that the offset has to be const, whereas the value does not
> have this requirement, so this makes perfect sense to me.
> 
> (I agree that it can look unfamiliar at the first glance though.)

Yes the familiarity is the issue. I still do not feel using a generic for this looks ok to me and I think we can fix it differently, I will try to come up with an alternative fix unless we have already decided to use generics for this.

> 
>> Sorry if I knew this would be the syntax, I would have objected even when we
>> spoke :)
>> 
>> I think the best fix (from any I've seen so far), is to move the bindings
>> calls of offending code into a closure and call the closure directly, as I
>> posted in the other thread. I also passed the closure idea by Gary and he
>> confirmed the compiler should behave correctly (I will check the code gen
>> with/without later). Gary also provided a brilliant suggestion that we can
>> call the closure directly instead of assigning it to a variable first. That
>> fix is also smaller, and does not screw up the users. APIs should fix issues
>> within them instead of relying on user to work around them.
> 
> This is not a workaround, this is an idiomatic solution (which I probably should
> have been doing already when I introduced the I/O code).
> 
> We do exactly the same thing for DmaMask::new() [1] and we agreed on doing the
> same thing for Alignment as well [2].
> 
> [1] https://rust.docs.kernel.org/kernel/dma/struct.DmaMask.html#method.new
> [2] https://lore.kernel.org/rust-for-linux/20250908-num-v5-1-c0f2f681ea96@nvidia.com/

Ah ok. Since there is precedent, I am ok with it, especially since you feel so strongly about it and since you are the rust IO code maintainer.

But passing one parameter as a constant generic and another parameter as a function parameter, just seems weird to me even if there is precedent.

And one can argue that the value is also a constant right? It is confusing and makes callers unreadable IMHO.

Cheers,

 - Joel