Extend Page to support pages that are not allocated by the constructor,
for example, those returned by vmalloc_to_page() or virt_to_page().
Since we don't own those pages we shouldn't Drop them either. Hence we
take advantage of the switch to Opaque so we can cast to a Page pointer
from a struct page pointer and be able to retrieve the reference on an
existing struct page mapping. In this case no destructor will be called
since we are not instantiating a new Page instance.
The new page_slice_to_page wrapper ensures that it explicity accepts
only page-sized chunks.
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
rust/helpers/page.c | 10 +++++
rust/kernel/page.rs | 97 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 106 insertions(+), 1 deletion(-)
diff --git a/rust/helpers/page.c b/rust/helpers/page.c
index 48d4481c1e33..784563924b83 100644
--- a/rust/helpers/page.c
+++ b/rust/helpers/page.c
@@ -27,3 +27,13 @@ void rust_helper_get_page(struct page *page)
{
get_page(page);
}
+
+struct page *rust_helper_virt_to_page(const void *kaddr)
+{
+ return virt_to_page(kaddr);
+}
+
+bool rust_helper_virt_addr_valid(const void *kaddr)
+{
+ return virt_addr_valid(kaddr);
+}
diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index fdf7ee203597..d0a896f53afb 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -3,7 +3,7 @@
//! Kernel page allocation and management.
use crate::{
- alloc::{AllocError, Flags},
+ alloc::{AllocError, Allocator, Flags, VVec, KVec, KVVec, Vec, flags::*},
bindings,
error::code::*,
error::Result,
@@ -87,6 +87,49 @@ pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })
}
+ /// Create a page object from a buffer which is associated with an existing C `struct page`.
+ ///
+ /// This function ensures it takes a page-sized buffer as represented by `PageSlice`.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::page::*;
+ ///
+ /// let somedata: [u8; PAGE_SIZE * 2] = [0; PAGE_SIZE * 2];
+ /// let buf: &[u8] = &somedata;
+ /// let pages: VVec<PageSlice> = buf.try_into()?;
+ /// let page = Page::page_slice_to_page(&pages[0])?;
+ /// # Ok::<(), Error>(())
+ /// ```
+ pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>
+ {
+ let ptr: *const core::ffi::c_void = page.0.as_ptr() as _;
+ if ptr.is_null() {
+ return Err(EINVAL)
+ }
+ // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
+ let page = if unsafe { bindings::is_vmalloc_addr(ptr) } {
+ // SAFETY: We've checked that `ptr` is non-null and within the vmalloc range, hence
+ // it's safe to call this method.
+ unsafe { bindings::vmalloc_to_page(ptr) }
+ // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
+ } else if unsafe { bindings::virt_addr_valid(ptr) } {
+ // SAFETY: We've checked that `ptr` is non-null and a valid virtual address, hence
+ // it's safe to call this method.
+ unsafe { bindings::virt_to_page(ptr) }
+ } else {
+ ptr::null_mut()
+ };
+ if page.is_null() {
+ return Err(EINVAL);
+ }
+ // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
+ // SAFETY: We just successfully retrieved an existing `bindings::page`, therefore
+ // dereferencing the page pointer is valid.
+ Ok(unsafe { &*page.cast() })
+ }
+
/// Returns a raw pointer to the page.
pub fn as_ptr(&self) -> *mut bindings::page {
self.page.get()
@@ -270,3 +313,55 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
unsafe { bindings::put_page(obj.cast().as_ptr()) }
}
}
+
+/// A page-aligned, page-sized object.
+///
+/// This is used for convenience to convert a large buffer into an array of page-sized chunks
+/// allocated with the kernel's allocators which can then be used in the
+/// `Page::page_slice_to_page` wrapper.
+///
+// FIXME: This should be `PAGE_SIZE`, but the compiler rejects everything except a literal
+// integer argument for the `repr(align)` attribute.
+#[repr(align(4096))]
+pub struct PageSlice([u8; PAGE_SIZE]);
+
+fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> {
+ let mut k = Vec::<PageSlice, A>::new();
+ let pages = page_align(val.len()) >> PAGE_SHIFT;
+ match k.reserve(pages, GFP_KERNEL) {
+ Ok(()) => {
+ // SAFETY: from above, the length should be equal to the vector's capacity
+ unsafe { k.set_len(pages); }
+ // SAFETY: src buffer sized val.len() does not overlap with dst buffer since
+ // the dst buffer's size is val.len() padded up to a multiple of PAGE_SIZE.
+ unsafe { ptr::copy_nonoverlapping(val.as_ptr(), k.as_mut_ptr() as *mut u8,
+ val.len()) };
+ Ok(k)
+ },
+ Err(_) => Err(AllocError),
+ }
+}
+
+impl TryFrom<&[u8]> for VVec<PageSlice> {
+ type Error = AllocError;
+
+ fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+ to_vec_with_allocator(val)
+ }
+}
+
+impl TryFrom<&[u8]> for KVec<PageSlice> {
+ type Error = AllocError;
+
+ fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+ to_vec_with_allocator(val)
+ }
+}
+
+impl TryFrom<&[u8]> for KVVec<PageSlice> {
+ type Error = AllocError;
+
+ fn try_from(val: &[u8]) -> Result<Self, AllocError> {
+ to_vec_with_allocator(val)
+ }
+}
--
2.43.0
Commenting as someone who understands kernel MM decently but only knows a tiny bit about Rust: On Tue, Nov 19, 2024 at 12:24 PM Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote: > Extend Page to support pages that are not allocated by the constructor, > for example, those returned by vmalloc_to_page() or virt_to_page(). > Since we don't own those pages we shouldn't Drop them either. Hence we > take advantage of the switch to Opaque so we can cast to a Page pointer > from a struct page pointer and be able to retrieve the reference on an > existing struct page mapping. In this case no destructor will be called > since we are not instantiating a new Page instance. > > The new page_slice_to_page wrapper ensures that it explicity accepts > only page-sized chunks. > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com> [...] > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs > index fdf7ee203597..d0a896f53afb 100644 > --- a/rust/kernel/page.rs > +++ b/rust/kernel/page.rs > @@ -3,7 +3,7 @@ > //! Kernel page allocation and management. > > use crate::{ > - alloc::{AllocError, Flags}, > + alloc::{AllocError, Allocator, Flags, VVec, KVec, KVVec, Vec, flags::*}, > bindings, > error::code::*, > error::Result, > @@ -87,6 +87,49 @@ pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> { > Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) }) > } > > + /// Create a page object from a buffer which is associated with an existing C `struct page`. > + /// > + /// This function ensures it takes a page-sized buffer as represented by `PageSlice`. > + /// > + /// # Examples > + /// > + /// ``` > + /// use kernel::page::*; > + /// > + /// let somedata: [u8; PAGE_SIZE * 2] = [0; PAGE_SIZE * 2]; > + /// let buf: &[u8] = &somedata; > + /// let pages: VVec<PageSlice> = buf.try_into()?; > + /// let page = Page::page_slice_to_page(&pages[0])?; > + /// # Ok::<(), Error>(()) > + /// ``` > + pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self> Sorry, can you explain to me what the semantics of this are? Does this create a Page reference that is not lifetime-bound to the PageSlice? > + { > + let ptr: *const core::ffi::c_void = page.0.as_ptr() as _; > + if ptr.is_null() { > + return Err(EINVAL) > + } > + // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method. > + let page = if unsafe { bindings::is_vmalloc_addr(ptr) } { > + // SAFETY: We've checked that `ptr` is non-null and within the vmalloc range, hence > + // it's safe to call this method. > + unsafe { bindings::vmalloc_to_page(ptr) } > + // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method. > + } else if unsafe { bindings::virt_addr_valid(ptr) } { > + // SAFETY: We've checked that `ptr` is non-null and a valid virtual address, hence > + // it's safe to call this method. > + unsafe { bindings::virt_to_page(ptr) } > + } else { > + ptr::null_mut() > + }; > + if page.is_null() { > + return Err(EINVAL); > + } > + // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`. > + // SAFETY: We just successfully retrieved an existing `bindings::page`, therefore > + // dereferencing the page pointer is valid. > + Ok(unsafe { &*page.cast() }) > + } > + > /// Returns a raw pointer to the page. > pub fn as_ptr(&self) -> *mut bindings::page { > self.page.get() > @@ -270,3 +313,55 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) { > unsafe { bindings::put_page(obj.cast().as_ptr()) } > } > } > + > +/// A page-aligned, page-sized object. > +/// > +/// This is used for convenience to convert a large buffer into an array of page-sized chunks > +/// allocated with the kernel's allocators which can then be used in the > +/// `Page::page_slice_to_page` wrapper. > +/// > +// FIXME: This should be `PAGE_SIZE`, but the compiler rejects everything except a literal > +// integer argument for the `repr(align)` attribute. > +#[repr(align(4096))] > +pub struct PageSlice([u8; PAGE_SIZE]); > + > +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> { > + let mut k = Vec::<PageSlice, A>::new(); > + let pages = page_align(val.len()) >> PAGE_SHIFT; > + match k.reserve(pages, GFP_KERNEL) { Do I understand correctly that this can be used to create a kmalloc allocation whose pages can then basically be passed to page_slice_to_page()? FYI, the page refcount does not protect against UAF of slab allocations through new slab allocations of the same size. In other words: The slab allocator can internally recycle memory without going through the page allocator, and the slab allocator itself does not care about page refcounts. If the Page returned from calling page_slice_to_page() on the slab memory pages returned from to_vec_with_allocator() is purely usable as a borrow and there is no way to later grab a refcounted reference to it or pass it into a C function that assumes it can grab a reference to the page, I guess that works. But if I understand correctly what's going on here, and you can grab references to slab pages returned from page_slice_to_page(to_vec_with_allocator()), that'd be bad. > + Ok(()) => { > + // SAFETY: from above, the length should be equal to the vector's capacity > + unsafe { k.set_len(pages); } > + // SAFETY: src buffer sized val.len() does not overlap with dst buffer since > + // the dst buffer's size is val.len() padded up to a multiple of PAGE_SIZE. > + unsafe { ptr::copy_nonoverlapping(val.as_ptr(), k.as_mut_ptr() as *mut u8, > + val.len()) }; > + Ok(k) > + }, > + Err(_) => Err(AllocError), > + } > +}
Hi, Thanks for the feedback. On 19/11/2024 19:07, Jann Horn wrote: >> + pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self> > > Sorry, can you explain to me what the semantics of this are? Does this > create a Page reference that is not lifetime-bound to the PageSlice? This creates a Page reference that is tied to the lifetime of the `C struct page` behind the PageSlice buffer. Basically, it's just a cast from the struct page pointer and does not own that resource. >> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> { > Do I understand correctly that this can be used to create a kmalloc > allocation whose pages can then basically be passed to > page_slice_to_page()? > > FYI, the page refcount does not protect against UAF of slab > allocations through new slab allocations of the same size. In other > words: The slab allocator can internally recycle memory without going > through the page allocator, and the slab allocator itself does not > care about page refcounts. > > If the Page returned from calling page_slice_to_page() on the slab > memory pages returned from to_vec_with_allocator() is purely usable as > a borrow and there is no way to later grab a refcounted reference to > it or pass it into a C function that assumes it can grab a reference > to the page, I guess that works. Yes, I think that is the intent. I appreciate your help in pointing out the issues with using refcounts in slab memory pages. As you can see, page_slice_to_page() only returns a Page reference (not a refcounted Page). Hopefully that addresses your concern? Regards, Abdiel
On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote: > On 19/11/2024 19:07, Jann Horn wrote: > >> + pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self> > > > > Sorry, can you explain to me what the semantics of this are? Does this > > create a Page reference that is not lifetime-bound to the PageSlice? > > This creates a Page reference that is tied to the lifetime of the `C > struct page` behind the PageSlice buffer. Basically, it's just a cast > from the struct page pointer and does not own that resource. How is the Page reference tied to the lifetime of the C "struct page"? I asked some Rust experts to explain to me what this method signature expands to, and they added the following to the Rust docs: https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md ``` fn other_args1<'a>(arg: &str) -> &'a str; // elided fn other_args2<'a, 'b>(arg: &'b str) -> &'a str; // expanded ``` Basically, my understanding is that since you are explicitly specifying that the result should have lifetime 'a, but you are not specifying the lifetime of the parameter, the parameter is given a separate, unrelated lifetime by the compiler? Am I misunderstanding how this works, or is that a typo in the method signature? > >> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> { > > Do I understand correctly that this can be used to create a kmalloc > > allocation whose pages can then basically be passed to > > page_slice_to_page()? > > > > FYI, the page refcount does not protect against UAF of slab > > allocations through new slab allocations of the same size. In other > > words: The slab allocator can internally recycle memory without going > > through the page allocator, and the slab allocator itself does not > > care about page refcounts. > > > > If the Page returned from calling page_slice_to_page() on the slab > > memory pages returned from to_vec_with_allocator() is purely usable as > > a borrow and there is no way to later grab a refcounted reference to > > it or pass it into a C function that assumes it can grab a reference > > to the page, I guess that works. > > Yes, I think that is the intent. I appreciate your help in pointing out > the issues with using refcounts in slab memory pages. As you can see, > page_slice_to_page() only returns a Page reference (not a refcounted > Page). Hopefully that addresses your concern? Does Rust also prevent safe code from invoking inc_ref() on the returned Page reference? Normally, the AlwaysRefCounted trait means that safe code can create an owned reference from a shared reference, right?
On 21/11/2024 22:17, Jann Horn wrote: > > Does Rust also prevent safe code from invoking inc_ref() on the > returned Page reference? Normally, the AlwaysRefCounted trait means > that safe code can create an owned reference from a shared reference, > right? While it is possible for someone to *manually* convert the Page reference returned in page_slice_to_page() to a refcounted Page (one could wrap it in an ARef). However, by design, page_slice_to_page() explicitly returns just an ordinary Page reference. We could add an invariant in page_slice_to_page() to warn against such usage just in case. Anyway seems like the consensus from the other thread is to avoid refcounting the rust Page abstraction. If we go with that, then that moots this issue. Regards, Abdiel
On Thu, Nov 21, 2024 at 9:18 PM Jann Horn <jannh@google.com> wrote: > > On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue > <abdiel.janulgue@gmail.com> wrote: > > On 19/11/2024 19:07, Jann Horn wrote: > > >> + pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self> > > > > > > Sorry, can you explain to me what the semantics of this are? Does this > > > create a Page reference that is not lifetime-bound to the PageSlice? > > > > This creates a Page reference that is tied to the lifetime of the `C > > struct page` behind the PageSlice buffer. Basically, it's just a cast > > from the struct page pointer and does not own that resource. > > How is the Page reference tied to the lifetime of the C "struct page"? > > I asked some Rust experts to explain to me what this method signature > expands to, and they added the following to the Rust docs: > > https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md > ``` > fn other_args1<'a>(arg: &str) -> &'a str; // elided > fn other_args2<'a, 'b>(arg: &'b str) -> &'a str; // expanded > ``` > > Basically, my understanding is that since you are explicitly > specifying that the result should have lifetime 'a, but you are not > specifying the lifetime of the parameter, the parameter is given a > separate, unrelated lifetime by the compiler? Am I misunderstanding > how this works, or is that a typo in the method signature? No, you are correct. The signature is wrong and lets the caller pick any lifetime they want, with no relation to the lifetime of the underlying `struct page`. From a C perspective, what are the lifetime requirements of vmalloc_to_page? > > >> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> { > > > Do I understand correctly that this can be used to create a kmalloc > > > allocation whose pages can then basically be passed to > > > page_slice_to_page()? > > > > > > FYI, the page refcount does not protect against UAF of slab > > > allocations through new slab allocations of the same size. In other > > > words: The slab allocator can internally recycle memory without going > > > through the page allocator, and the slab allocator itself does not > > > care about page refcounts. > > > > > > If the Page returned from calling page_slice_to_page() on the slab > > > memory pages returned from to_vec_with_allocator() is purely usable as > > > a borrow and there is no way to later grab a refcounted reference to > > > it or pass it into a C function that assumes it can grab a reference > > > to the page, I guess that works. > > > > Yes, I think that is the intent. I appreciate your help in pointing out > > the issues with using refcounts in slab memory pages. As you can see, > > page_slice_to_page() only returns a Page reference (not a refcounted > > Page). Hopefully that addresses your concern? > > Does Rust also prevent safe code from invoking inc_ref() on the > returned Page reference? Normally, the AlwaysRefCounted trait means > that safe code can create an owned reference from a shared reference, > right? In principle, no, you could invoke inc_ref() directly. But the correct way to do it would be to use ARef::from(my_ref) to obtain an actual refcounted reference. Alice
On 22/11/2024 09:55, Alice Ryhl wrote: > On Thu, Nov 21, 2024 at 9:18 PM Jann Horn <jannh@google.com> wrote: >> >> On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue >> <abdiel.janulgue@gmail.com> wrote: >>> On 19/11/2024 19:07, Jann Horn wrote: >>>>> + pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self> >>>> >>>> Sorry, can you explain to me what the semantics of this are? Does this >>>> create a Page reference that is not lifetime-bound to the PageSlice? >>> >>> This creates a Page reference that is tied to the lifetime of the `C >>> struct page` behind the PageSlice buffer. Basically, it's just a cast >>> from the struct page pointer and does not own that resource. >> >> How is the Page reference tied to the lifetime of the C "struct page"? >> >> I asked some Rust experts to explain to me what this method signature >> expands to, and they added the following to the Rust docs: >> >> https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md >> ``` >> fn other_args1<'a>(arg: &str) -> &'a str; // elided >> fn other_args2<'a, 'b>(arg: &'b str) -> &'a str; // expanded >> ``` >> >> Basically, my understanding is that since you are explicitly >> specifying that the result should have lifetime 'a, but you are not >> specifying the lifetime of the parameter, the parameter is given a >> separate, unrelated lifetime by the compiler? Am I misunderstanding >> how this works, or is that a typo in the method signature? > > No, you are correct. The signature is wrong and lets the caller pick > any lifetime they want, with no relation to the lifetime of the > underlying `struct page`. But that could be put in the invariant that the PageSlice buffer must last at least the lifetime `'a`? > > From a C perspective, what are the lifetime requirements of vmalloc_to_page? > If I'm not mistaken, that should be the lifetime of the vmalloc'd buffer right?
On Fri, Nov 22, 2024 at 9:36 AM Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote: > > On 22/11/2024 09:55, Alice Ryhl wrote: > > On Thu, Nov 21, 2024 at 9:18 PM Jann Horn <jannh@google.com> wrote: > >> > >> On Wed, Nov 20, 2024 at 11:56 PM Abdiel Janulgue > >> <abdiel.janulgue@gmail.com> wrote: > >>> On 19/11/2024 19:07, Jann Horn wrote: > >>>>> + pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self> > >>>> > >>>> Sorry, can you explain to me what the semantics of this are? Does this > >>>> create a Page reference that is not lifetime-bound to the PageSlice? > >>> > >>> This creates a Page reference that is tied to the lifetime of the `C > >>> struct page` behind the PageSlice buffer. Basically, it's just a cast > >>> from the struct page pointer and does not own that resource. > >> > >> How is the Page reference tied to the lifetime of the C "struct page"? > >> > >> I asked some Rust experts to explain to me what this method signature > >> expands to, and they added the following to the Rust docs: > >> > >> https://github.com/rust-lang/reference/blob/master/src/lifetime-elision.md > >> ``` > >> fn other_args1<'a>(arg: &str) -> &'a str; // elided > >> fn other_args2<'a, 'b>(arg: &'b str) -> &'a str; // expanded > >> ``` > >> > >> Basically, my understanding is that since you are explicitly > >> specifying that the result should have lifetime 'a, but you are not > >> specifying the lifetime of the parameter, the parameter is given a > >> separate, unrelated lifetime by the compiler? Am I misunderstanding > >> how this works, or is that a typo in the method signature? > > > > No, you are correct. The signature is wrong and lets the caller pick > > any lifetime they want, with no relation to the lifetime of the > > underlying `struct page`. > > But that could be put in the invariant that the PageSlice buffer must > last at least the lifetime `'a`? > > > > > From a C perspective, what are the lifetime requirements of vmalloc_to_page? > > > > If I'm not mistaken, that should be the lifetime of the vmalloc'd buffer > right? It seems to me that the signature should look like this: fn vmalloc_to_page(vec: &VVec<PageSlice>, i: usize) -> &Page This way, by providing the VVec, you can only use it with memory that really comes from a vmalloc call. Alice
© 2016 - 2024 Red Hat, Inc.