| 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
| ||
|
Message-ID: <4bfba51907578cc0f4f25368240720f4148a2736.camel@posteo.de>
Date: Mon, 13 Oct 2025 13:43:57 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Benno Lossin <lossin@...nel.org>, Danilo Krummrich <dakr@...nel.org>,
Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Lee
Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Vlastimil Babka
<vbabka@...e.cz>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, Uladzislau
Rezki <urezki@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
<gary@...yguo.net>, bjorn3_gh@...tonmail.com, Andreas Hindborg
<a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross
<tmgross@...ch.edu>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v4 1/2] rust: add basic Pin<Vec<T, A>> abstractions
On Mon, 2025-10-13 at 10:03 +0200, Benno Lossin wrote:
> On Mon Oct 13, 2025 at 12:11 AM CEST, Markus Probst wrote:
> > On Sun, 2025-10-12 at 23:31 +0200, Benno Lossin wrote:
> > > On Sun Oct 12, 2025 at 6:57 PM CEST, Markus Probst wrote:
> > > > From what I can tell, there is no way to get a `Pin<&mut Vec<T,
> > > > A>>`
> > > > from a `&mut Pin<Vec<T, A>>`. We can only get `Pin<&mut [T]>`
> > > > which
> > > > is
> > > > not usable in our case.
> > >
> > > Hmm yeah that's true.
> > >
> > > > If there is way, without the extension trait or an extra
> > > > struct, I
> > > > would be happy to implement it.
> > >
> > > So I tried to look for the usage site of this and I found this
> > > usage
> > > in
> > > your v1:
> > >
> > > + let mut leds = KPinnedVec::with_capacity(
> > > + Atmega1608LedAddress::VALUES.len() *
> > > Atmega1608LedId::VALUES.len(),
> > > + GFP_KERNEL,
> > > + )?;
> > > +
> > > + let mut i = 0;
> > > + for addr in Atmega1608LedAddress::VALUES {
> > > + let mode_lock = Arc::pin_init(new_mutex!(()),
> > > GFP_KERNEL)?;
> > > +
> > > + for id in Atmega1608LedId::VALUES {
> > > + let Some(child) =
> > > +
> > > fwnode.get_child_by_name(&CString::try_from_fmt(fmt!("led@{i}"))?
> > > )
> > > + else {
> > > + continue;
> > > + };
> > > +
> > > + let client = ARef::clone(&client);
> > > + let mode_lock = Arc::clone(&mode_lock);
> > > +
> > > + leds.push_pin_init(LedClassDev::new(
> > > + Some(idev),
> > > + None,
> > > + LedInitData::new().fwnode(&child),
> > > + Atmega1608Led {
> > > + addr,
> > > + id,
> > > + client,
> > > +
> > > + mode_lock,
> > > + },
> > > + ))?;
> > > + i += 1;
> > > + }
> > > + }
> > > + Ok(KBox::new(Self { client, leds },
> > > GFP_KERNEL)?.into())
> > >
> > > And I think using `Vec` for this is just wrong. `Vec` is a data
> > > structure that supports growing and shrinking the allocation. But
> > > you
> > > just need a fixed size buffer that holds all your data. Do you
> > > think
> > > that `Pin<Box<[LedClassDev]>>` would suffice if it had proper
> > > support
> > > from pin-init?
> > As you can see in v1, the number of leds (or vec entries) depends
> > on
> > the fwnode (see the continue statement there). I don't think that
> > counts as fixed size. `Pin<KBox<[Option<LedClassDev>]>>` could
> > potentially be used instead of `Pin<KVec<LedClassDev>>` in my
> > scenario,
> > but that would require an extra byte of allocation for the max leds
> > of
> > 24 each and the code would look more ugly. At the point I use
> > Option in
> > the slice, its basically an unoptimized Vec (instead of storing the
> > length, it stores if an item in the buffer is present or not).
>
> You can just make the length of the slice be the desired length?
That would work, but creates another allocation on the heap (Vec<I>)
that could have been avoided. I don't think it would make `Pin<Vec<T,
A>>` obsolete.
Or would you rather say, such allocations don't matter?
> (also,
> `i` is never incremented in the `continue` case, so it will act like
> a
> `break`?)
You just found a bug in v1.
Thanks
- Markus Probst
[1] https://docs.rs/arrayvec/latest/arrayvec/struct.ArrayString.html
>
> One option that we have would be storing the initializers in a vec:
>
> fn probe(
> pdev: &I2cClient<kernel::device::Core>,
> _id_info: Option<&Self::IdInfo>,
> ) -> Result<Pin<KBox<Self>>> {
> let idev = pdev.as_ref();
>
> let Some(fwnode) = idev.fwnode() else {
> return Err(EINVAL);
> };
>
> let client: ARef<I2cClient> = pdev.into();
>
> client
> .write_byte_data(1, 0)
> .inspect_err(|err| dev_err!(idev, "unable to remove led
> mask: {err:?}\n"))?;
>
> let mut led_init = KVec::new();
>
> let mut i = 0;
> for addr in Atmega1608LedAddress::VALUES {
> let mode_lock = Arc::pin_init(new_mutex!(()),
> GFP_KERNEL)?;
>
> for id in Atmega1608LedId::VALUES {
> let Some(child) =
>
> fwnode.get_child_by_name(&CString::try_from_fmt(fmt!("led@{i}"))?)
> else {
> continue;
> };
>
> let client = ARef::clone(&client);
> let mode_lock = Arc::clone(&mode_lock);
>
> led_init.push(LedClassDev::new(
> Some(idev),
> None,
> LedInitData::new().fwnode(&child),
> Atmega1608Led {
> addr,
> id,
> client,
> mode_lock,
> },
> ))?;
> i += 1;
> }
> }
> let leds = Vec::pin_init_slice(led_init, GFP_KERNEL)?;
> Ok(KBox::new(Self { client, leds }, GFP_KERNEL)?.into())
> }
>
> And `Vec::pin_init_slice` would have the following signature:
>
> fn pin_init_slice<T, I, E>(this: Vec<I>, flags: alloc::Flags) ->
> Result<Pin<Box<[T]>>>
> where
> I: PinInit<T, E>,
> Error: From<E>;
>
> ---
> Cheers,
> Benno
>
> >
> > >
> > > Also, please don't top-post [1] and take a look at your mail
> > > client
> > > configuration, it puts lots of extra `> ` at the end which looks
> > > pretty
> > > strange [2].
> > Yes, I did notice that. It is not present when writing a reply, but
> > after it got sent for some reason (most replies, not all). It is
> > GNOME
> > Evolution in its default settings basically. My distro ships a 4
> > months
> > outdated version (3.56.2), which shouldn't be too old, but I will
> > investiage.
> >
> > Thanks
> > - Markus Probst
> > >
> > > [1]:
> > > https://docs.kernel.org/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions
> > > [2]:
> > > https://lore.kernel.org/all/e550b0862e9ea87e50688d1ec8f623638d170a3a.camel@posteo.de
> > >
> > > ---
> > > Cheers,
> > > Benno
Powered by blists - more mailing lists