| Message ID | 20241119112408.779243-3-abdiel.janulgue@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: page: Add support for existing struct page mappings | expand |
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
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) + } +}
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(-)