| 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
| ||
|
Date: Tue, 26 Mar 2024 22:54:42 -0300
From: Wedson Almeida Filho <wedsonaf@...il.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: rust-for-linux@...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@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>,
linux-kernel@...r.kernel.org, Wedson Almeida Filho <walmeida@...rosoft.com>
Subject: Re: [PATCH 06/10] rust: alloc: introduce the `BoxExt` trait
On Tue, 26 Mar 2024 at 10:30, Benno Lossin <benno.lossin@...ton.me> wrote:
>
> On 26.03.24 01:17, Wedson Almeida Filho wrote:
> > On Mon, 25 Mar 2024 at 19:37, Benno Lossin <benno.lossin@...ton.me> wrote:
> >>
> >> On 25.03.24 20:54, Wedson Almeida Filho wrote:
> >>> From: Wedson Almeida Filho <walmeida@...rosoft.com>
> >>>
> >>> Make fallible versions of `new` and `new_uninit` methods available in
> >>> `Box` even though it doesn't implement them because we build `alloc`
> >>> with the `no_global_oom_handling` config.
> >>>
> >>> They also have an extra `flags` parameter that allows callers to pass
> >>> flags to the allocator.
> >>>
> >>> Signed-off-by: Wedson Almeida Filho <walmeida@...rosoft.com>
> >>> ---
> >>> rust/kernel/alloc.rs | 1 +
> >>> rust/kernel/alloc/allocator.rs | 6 +++-
> >>> rust/kernel/alloc/boxext.rs | 61 ++++++++++++++++++++++++++++++++++
> >>> rust/kernel/init.rs | 13 ++++----
> >>> rust/kernel/prelude.rs | 2 +-
> >>> rust/kernel/sync/arc.rs | 3 +-
> >>> 6 files changed, 77 insertions(+), 9 deletions(-)
> >>> create mode 100644 rust/kernel/alloc/boxext.rs
> >>>
> >>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> >>> index ad48ac8dc13d..5712c81b1308 100644
> >>> --- a/rust/kernel/alloc.rs
> >>> +++ b/rust/kernel/alloc.rs
> >>> @@ -5,6 +5,7 @@
> >>> #[cfg(not(test))]
> >>> #[cfg(not(testlib))]
> >>> mod allocator;
> >>> +pub mod boxext;
> >>> pub mod vecext;
>
> One thing I forgot to say: I think these modules should be named
> `box_ext` and `vec_ext`. It fits better with the usual style.
Yeah, looking at std for precedents, I see one module called
`backtrace` but also `assert_matches` and `async_iter`.
Snake case is the standard anyway, so I'll make the change for v2.
>
> >>>
> >>> /// Flags to be used when allocating memory.
> >>> diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
> >>> index 01ad139e19bc..fc0439455faa 100644
> >>> --- a/rust/kernel/alloc/allocator.rs
> >>> +++ b/rust/kernel/alloc/allocator.rs
> >>> @@ -15,7 +15,11 @@
> >>> ///
> >>> /// - `ptr` can be either null or a pointer which has been allocated by this allocator.
> >>> /// - `new_layout` must have a non-zero size.
> >>> -unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: bindings::gfp_t) -> *mut u8 {
> >>> +pub(crate) unsafe fn krealloc_aligned(
> >>> + ptr: *mut u8,
> >>> + new_layout: Layout,
> >>> + flags: bindings::gfp_t,
> >>> +) -> *mut u8 {
> >>> // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
> >>> let layout = new_layout.pad_to_align();
> >>>
> >>> diff --git a/rust/kernel/alloc/boxext.rs b/rust/kernel/alloc/boxext.rs
> >>> new file mode 100644
> >>> index 000000000000..26a918df7acf
> >>> --- /dev/null
> >>> +++ b/rust/kernel/alloc/boxext.rs
> >>> @@ -0,0 +1,61 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +//! Extensions to [`Box`] for fallible allocations.
> >>> +
> >>> +use super::Flags;
> >>> +use alloc::boxed::Box;
> >>> +use core::alloc::AllocError;
> >>> +use core::mem::MaybeUninit;
> >>> +use core::result::Result;
> >>> +
> >>> +/// Extensions to [`Box`].
> >>> +pub trait BoxExt<T>: Sized {
> >>> + /// Allocates a new box.
> >>> + ///
> >>> + /// The allocation may fail, in which case an error is returned.
> >>> + fn new(x: T, flags: Flags) -> Result<Self, AllocError>;
> >>> +
> >>> + /// Allocates a new uninitialised box.
> >>> + ///
> >>> + /// The allocation may fail, in which case an error is returned.
> >>> + fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
> >>> +}
> >>> +
> >>> +impl<T> BoxExt<T> for Box<T> {
> >>> + #[cfg(any(test, testlib))]
> >>> + fn new(x: T, _flags: Flags) -> Result<Self, AllocError> {
> >>> + Ok(Box::new(x))
> >>> + }
> >>
> >> When running under `cfg(test)`, are we using the normal standard
> >> library? Or why is this needed?
> >
> > Because it uses 34 other crates that rely on `Box::new` and friends.
> >
> > I discussed this with Miguel recently and once he's done with the
> > build system changes, he will think about what to do with tests. It
> > may be that we abandon the current method of running standalone tests
> > and run everything in kunit, or perhaps we'll find a way to exclude
> > code that won't run in standalone tests anyway...
>
> Ah I see, I think it would be nice to not need this. Let's see what the
> new build system can do here.
>
> >
> >>> +
> >>> + #[cfg(not(any(test, testlib)))]
> >>> + fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
> >>> + let ptr = if core::mem::size_of::<T>() == 0 {
> >>> + core::ptr::NonNull::<T>::dangling().as_ptr()
> >>> + } else {
> >>> + let layout = core::alloc::Layout::new::<T>();
> >>> +
> >>> + // SAFETY: Memory is being allocated (first arg is null). The only other source of
> >>> + // safety issues is sleeping on atomic context, which is addressed by klint.
> >>
> >> The `krealloc_aligned` function states:
> >>
> >> /// # Safety
> >> ///
> >> /// - `ptr` can be either null or a pointer which has been allocated by this allocator.
> >> /// - `new_layout` must have a non-zero size.
> >>
> >> So it should also mention that you checked for `layout.size() > 0`
> >> above.
> >
> > Good point. I mentioned this in the VecExt version but not here. I
> > will update this for v2.
> >
> >>> + let ptr = unsafe {
> >>> + super::allocator::krealloc_aligned(core::ptr::null_mut(), layout, flags.0)
> >>> + };
> >>> + if ptr.is_null() {
> >>> + return Err(AllocError);
> >>> + }
> >>> +
> >>> + let ptr = ptr.cast::<T>();
> >>> +
> >>> + // SAFETY: We just allocated the memory above, it is valid for write.
> >>> + unsafe { ptr.write(x) };
> >>> + ptr
> >>> + };
> >>> +
> >>> + // SAFETY: For non-zero-sized types, we allocate above using the global allocator. For
> >>> + // zero-sized types, we use `NonNull::dangling`.
> >>> + Ok(unsafe { Box::from_raw(ptr) })
> >>> + }
> >>> +
> >>> + fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
> >>> + <Box<_> as BoxExt<_>>::new(MaybeUninit::<T>::uninit(), flags)
> >>
> >> Why do you use the extended syntax? I tried to use `Box::new` and it
> >> compiled.
> >
> > It works when compiling the kernel but fails when compiling for
> > userspace with regular (no_global_oom_handling disabled) `alloc` when
> > running `make rusttest`. In the latter case, it chooses the inherent
> > version of `Box::new` which is infallible and doesn't take flags so it
> > fails to compile.
> >
> > Using the extended syntax allows it always pick the right version,
> > regardless of how `alloc` is compiled.
> >
> > There are 5 places in existing code that required this change and this
> > is limited to the kernel crate (e.g., drivers, samples and
> > documentation examples can continue to use `Box::new`). So we thought
> > it was ok until Miguel figures out what we want to do with tests.
>
> Thanks for the explanation, again it would be nice to be able to just
> write `Box::new`.
>
> --
> Cheers,
> Benno
>
Powered by blists - more mailing lists