| Message ID | 20241120-vma-v8-3-eb31425da66b@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rust support for mm_struct, vm_area_struct, and mmap | expand |
On Wed, Nov 20, 2024 at 02:49:57PM +0000, Alice Ryhl wrote: > The vm_insert_page method is only usable on vmas with the VM_MIXEDMAP > flag, so we introduce a new type to keep track of such vmas. Worth mentioning that it can be used for VMAs without this flag set, but if so we must hold a _write_ lock to be able to do so, so it can update the flag itself, however we intend only to use it with VMAs which already have this flag set. > > The approach used in this patch assumes that we will not need to encode > many flag combinations in the type. I don't think we need to encode more > than VM_MIXEDMAP and VM_PFNMAP as things are now. However, if that > becomes necessary, using generic parameters in a single type would scale > better as the number of flags increases. > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > rust/kernel/mm/virt.rs | 68 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 67 insertions(+), 1 deletion(-) > > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs > index 1e755dca46dd..de7f2338810a 100644 > --- a/rust/kernel/mm/virt.rs > +++ b/rust/kernel/mm/virt.rs > @@ -4,7 +4,14 @@ > > //! Virtual memory. > > -use crate::{bindings, types::Opaque}; > +use crate::{ > + bindings, > + error::{to_result, Result}, > + page::Page, > + types::Opaque, > +}; > + > +use core::ops::Deref; > > /// A wrapper for the kernel's `struct vm_area_struct` with read access. > /// > @@ -80,6 +87,65 @@ pub fn zap_page_range_single(&self, address: usize, size: usize) { > ) > }; > } > + > + /// Check whether the `VM_MIXEDMAP` flag is set. > + #[inline] > + pub fn check_mixedmap(&self) -> Option<&VmAreaMixedMap> { Nitty + a little bikesheddy (this may be my mistake as I am unfamiliar with rust idioms also) but shouldn't this be 'as_mixedmap_vma()' or something? > + if self.flags() & flags::MIXEDMAP != 0 { > + // SAFETY: We just checked that `VM_MIXEDMAP` is set. All other requirements are > + // satisfied by the type invariants of `VmAreaRef`. > + Some(unsafe { VmAreaMixedMap::from_raw(self.as_ptr()) }) > + } else { > + None > + } > + } > +} > + > +/// A wrapper for the kernel's `struct vm_area_struct` with read access and `VM_MIXEDMAP` set. > +/// > +/// It represents an area of virtual memory. > +/// > +/// # Invariants > +/// > +/// The caller must hold the mmap read lock or the vma read lock. The `VM_MIXEDMAP` flag must be > +/// set. > +#[repr(transparent)] > +pub struct VmAreaMixedMap { > + vma: VmAreaRef, > +} > + > +// Make all `VmAreaRef` methods available on `VmAreaMixedMap`. > +impl Deref for VmAreaMixedMap { > + type Target = VmAreaRef; > + > + #[inline] > + fn deref(&self) -> &VmAreaRef { > + &self.vma > + } > +} Ah OK, thinking back to the 'impl Deref' from the other patch, I guess this allows you to deref VmAreaMixedMap as a VmAreaRef, does it all sort of automagically merge methods for you as if they were mix-ins then? > + > +impl VmAreaMixedMap { > + /// Access a virtual memory area given a raw pointer. > + /// > + /// # Safety > + /// > + /// Callers must ensure that `vma` is valid for the duration of 'a, and that the mmap read lock > + /// (or stronger) is held for at least the duration of 'a. The `VM_MIXEDMAP` flag must be set. > + #[inline] > + pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -> &'a Self { > + // SAFETY: The caller ensures that the invariants are satisfied for the duration of 'a. > + unsafe { &*vma.cast() } > + } > + > + /// Maps a single page at the given address within the virtual memory area. > + /// > + /// This operation does not take ownership of the page. > + #[inline] > + pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result { I'm guessing this 'Result' type encodes 0 vs. -Exxx error codes? > + // SAFETY: The caller has read access and has verified that `VM_MIXEDMAP` is set. The page > + // is order 0. The address is checked on the C side so it can take any value. > + to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address as _, page.as_ptr()) }) > + } > } It's really nice to abstract this as a seprate type and then to know its methods only apply in known circumstances... I guess in future we can use clever generic types when it comes to combinations of characteristics that would otherwise result in a combinatorial explosion? > > /// The integer type used for vma flags. > > -- > 2.47.0.371.ga323438b13-goog >
On Wed, Nov 20, 2024 at 8:20 PM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Wed, Nov 20, 2024 at 02:49:57PM +0000, Alice Ryhl wrote: > > The vm_insert_page method is only usable on vmas with the VM_MIXEDMAP > > flag, so we introduce a new type to keep track of such vmas. > > Worth mentioning that it can be used for VMAs without this flag set, but if > so we must hold a _write_ lock to be able to do so, so it can update the > flag itself, however we intend only to use it with VMAs which already have > this flag set. I got the impression from Jann that the automatically-set-VM_MIXEDMAP thing should only be used during mmap, and that VM_MIXEDMAP should not get set after initialization even with a write lock? https://lore.kernel.org/all/CAG48ez3gXicVYXiPsQDmYuPSsKMbES2KRQDk+0ANWSS0zDkqSw@mail.gmail.com/ > > The approach used in this patch assumes that we will not need to encode > > many flag combinations in the type. I don't think we need to encode more > > than VM_MIXEDMAP and VM_PFNMAP as things are now. However, if that > > becomes necessary, using generic parameters in a single type would scale > > better as the number of flags increases. > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > --- > > rust/kernel/mm/virt.rs | 68 +++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 67 insertions(+), 1 deletion(-) > > > > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs > > index 1e755dca46dd..de7f2338810a 100644 > > --- a/rust/kernel/mm/virt.rs > > +++ b/rust/kernel/mm/virt.rs > > @@ -4,7 +4,14 @@ > > > > //! Virtual memory. > > > > -use crate::{bindings, types::Opaque}; > > +use crate::{ > > + bindings, > > + error::{to_result, Result}, > > + page::Page, > > + types::Opaque, > > +}; > > + > > +use core::ops::Deref; > > > > /// A wrapper for the kernel's `struct vm_area_struct` with read access. > > /// > > @@ -80,6 +87,65 @@ pub fn zap_page_range_single(&self, address: usize, size: usize) { > > ) > > }; > > } > > + > > + /// Check whether the `VM_MIXEDMAP` flag is set. > > + #[inline] > > + pub fn check_mixedmap(&self) -> Option<&VmAreaMixedMap> { > > Nitty + a little bikesheddy (this may be my mistake as I am unfamiliar with > rust idioms also) but shouldn't this be 'as_mixedmap_vma()' or something? You're probably right that this name is more consistent with Rust naming conventions. :) > > + if self.flags() & flags::MIXEDMAP != 0 { > > + // SAFETY: We just checked that `VM_MIXEDMAP` is set. All other requirements are > > + // satisfied by the type invariants of `VmAreaRef`. > > + Some(unsafe { VmAreaMixedMap::from_raw(self.as_ptr()) }) > > + } else { > > + None > > + } > > + } > > +} > > + > > +/// A wrapper for the kernel's `struct vm_area_struct` with read access and `VM_MIXEDMAP` set. > > +/// > > +/// It represents an area of virtual memory. > > +/// > > +/// # Invariants > > +/// > > +/// The caller must hold the mmap read lock or the vma read lock. The `VM_MIXEDMAP` flag must be > > +/// set. > > +#[repr(transparent)] > > +pub struct VmAreaMixedMap { > > + vma: VmAreaRef, > > +} > > + > > +// Make all `VmAreaRef` methods available on `VmAreaMixedMap`. > > +impl Deref for VmAreaMixedMap { > > + type Target = VmAreaRef; > > + > > + #[inline] > > + fn deref(&self) -> &VmAreaRef { > > + &self.vma > > + } > > +} > > Ah OK, thinking back to the 'impl Deref' from the other patch, I guess this > allows you to deref VmAreaMixedMap as a VmAreaRef, does it all sort of > automagically merge methods for you as if they were mix-ins then? Yeah, I use it to "merge" the method sets to avoid duplication. The main limitation is that you can only deref to one other type, so you can't have "diamond inheritance". > > +impl VmAreaMixedMap { > > + /// Access a virtual memory area given a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure that `vma` is valid for the duration of 'a, and that the mmap read lock > > + /// (or stronger) is held for at least the duration of 'a. The `VM_MIXEDMAP` flag must be set. > > + #[inline] > > + pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -> &'a Self { > > + // SAFETY: The caller ensures that the invariants are satisfied for the duration of 'a. > > + unsafe { &*vma.cast() } > > + } > > + > > + /// Maps a single page at the given address within the virtual memory area. > > + /// > > + /// This operation does not take ownership of the page. > > + #[inline] > > + pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result { > > I'm guessing this 'Result' type encodes 0 vs. -Exxx error codes? In this particular case, yes. More generally, Result is a discriminated union containing either Ok(val) for success or Err(err) with some error type. But the default success type is the unit type () and the default error type is kernel::error::Error which corresponds to errno numbers. In this case, the compiler is clever enough to not use a discriminated union and instead represents Ok(()) as 0 and Err(err) as just the negative errno. This works since kernel::error::Error uses NonZeroI32 as its only field (as of 6.13). > > + // SAFETY: The caller has read access and has verified that `VM_MIXEDMAP` is set. The page > > + // is order 0. The address is checked on the C side so it can take any value. > > + to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address as _, page.as_ptr()) }) > > + } > > } > > It's really nice to abstract this as a seprate type and then to know its > methods only apply in known circumstances... I guess in future we can use > clever generic types when it comes to combinations of characteristics that > would otherwise result in a combinatorial explosion? Yeah, so the alternate approach I mention in the commit message would be to have something like this: struct VmAreaRef<const MIXEDMAP: bool, const PFNMAP: bool, const MAYWRITE: bool> { ... } listing all the flags we care about. This way, we get 2^3 different types by writing just one. (There are a few different variations on how to do this, instead of bools, we may want to allow three options: true, false, unknown.) Alice
diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs index 1e755dca46dd..de7f2338810a 100644 --- a/rust/kernel/mm/virt.rs +++ b/rust/kernel/mm/virt.rs @@ -4,7 +4,14 @@ //! Virtual memory. -use crate::{bindings, types::Opaque}; +use crate::{ + bindings, + error::{to_result, Result}, + page::Page, + types::Opaque, +}; + +use core::ops::Deref; /// A wrapper for the kernel's `struct vm_area_struct` with read access. /// @@ -80,6 +87,65 @@ pub fn zap_page_range_single(&self, address: usize, size: usize) { ) }; } + + /// Check whether the `VM_MIXEDMAP` flag is set. + #[inline] + pub fn check_mixedmap(&self) -> Option<&VmAreaMixedMap> { + if self.flags() & flags::MIXEDMAP != 0 { + // SAFETY: We just checked that `VM_MIXEDMAP` is set. All other requirements are + // satisfied by the type invariants of `VmAreaRef`. + Some(unsafe { VmAreaMixedMap::from_raw(self.as_ptr()) }) + } else { + None + } + } +} + +/// A wrapper for the kernel's `struct vm_area_struct` with read access and `VM_MIXEDMAP` set. +/// +/// It represents an area of virtual memory. +/// +/// # Invariants +/// +/// The caller must hold the mmap read lock or the vma read lock. The `VM_MIXEDMAP` flag must be +/// set. +#[repr(transparent)] +pub struct VmAreaMixedMap { + vma: VmAreaRef, +} + +// Make all `VmAreaRef` methods available on `VmAreaMixedMap`. +impl Deref for VmAreaMixedMap { + type Target = VmAreaRef; + + #[inline] + fn deref(&self) -> &VmAreaRef { + &self.vma + } +} + +impl VmAreaMixedMap { + /// Access a virtual memory area given a raw pointer. + /// + /// # Safety + /// + /// Callers must ensure that `vma` is valid for the duration of 'a, and that the mmap read lock + /// (or stronger) is held for at least the duration of 'a. The `VM_MIXEDMAP` flag must be set. + #[inline] + pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -> &'a Self { + // SAFETY: The caller ensures that the invariants are satisfied for the duration of 'a. + unsafe { &*vma.cast() } + } + + /// Maps a single page at the given address within the virtual memory area. + /// + /// This operation does not take ownership of the page. + #[inline] + pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result { + // SAFETY: The caller has read access and has verified that `VM_MIXEDMAP` is set. The page + // is order 0. The address is checked on the C side so it can take any value. + to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address as _, page.as_ptr()) }) + } } /// The integer type used for vma flags.
The vm_insert_page method is only usable on vmas with the VM_MIXEDMAP flag, so we introduce a new type to keep track of such vmas. The approach used in this patch assumes that we will not need to encode many flag combinations in the type. I don't think we need to encode more than VM_MIXEDMAP and VM_PFNMAP as things are now. However, if that becomes necessary, using generic parameters in a single type would scale better as the number of flags increases. Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- rust/kernel/mm/virt.rs | 68 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-)