| Message ID | 20241210-miscdevice-file-param-v3-2-b2a79b666dc5@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Additional miscdevice fops parameters | expand |
On Tue, 10 Dec 2024, Alice Ryhl wrote: > Providing access to the underlying `struct miscdevice` is useful for > various reasons. For example, this allows you access the miscdevice's > internal `struct device` for use with the `dev_*` printing macros. > > Note that since the underlying `struct miscdevice` could get freed at > any point after the fops->open() call (if misc_deregister is called), > only the open call is given access to it. To use `dev_*` printing macros > from other fops hooks, take a refcount on `miscdevice->this_device` to > keep it alive. See the linked thread for further discussion on the > lifetime of `struct miscdevice`. > > Link: https://lore.kernel.org/r/2024120951-botanist-exhale-4845@gregkh > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > rust/kernel/miscdevice.rs | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) Reviewed-by: Lee Jones <lee@kernel.org> > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs > index 0cb79676c139..75a9d26c8001 100644 > --- a/rust/kernel/miscdevice.rs > +++ b/rust/kernel/miscdevice.rs > @@ -97,14 +97,14 @@ fn drop(self: Pin<&mut Self>) { > > /// Trait implemented by the private data of an open misc device. > #[vtable] > -pub trait MiscDevice { > +pub trait MiscDevice: Sized { > /// What kind of pointer should `Self` be wrapped in. > type Ptr: ForeignOwnable + Send + Sync; > > /// Called when the misc device is opened. > /// > /// The returned pointer will be stored as the private data for the file. > - fn open(_file: &File) -> Result<Self::Ptr>; > + fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>; > > /// Called when the misc device is released. > fn release(device: Self::Ptr, _file: &File) { > @@ -182,24 +182,38 @@ impl<T: MiscDevice> VtableHelper<T> { > /// The file must be associated with a `MiscDeviceRegistration<T>`. > unsafe extern "C" fn fops_open<T: MiscDevice>( > inode: *mut bindings::inode, > - file: *mut bindings::file, > + raw_file: *mut bindings::file, > ) -> c_int { > // SAFETY: The pointers are valid and for a file being opened. > - let ret = unsafe { bindings::generic_file_open(inode, file) }; > + let ret = unsafe { bindings::generic_file_open(inode, raw_file) }; > if ret != 0 { > return ret; > } > > + // SAFETY: The open call of a file can access the private data. > + let misc_ptr = unsafe { (*raw_file).private_data }; > + > + // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the > + // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()` > + // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`. > + let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() }; > + > // SAFETY: > - // * The file is valid for the duration of this call. > + // * This underlying file is valid for (much longer than) the duration of `T::open`. > // * There is no active fdget_pos region on the file on this thread. > - let ptr = match T::open(unsafe { File::from_raw_file(file) }) { > + let file = unsafe { File::from_raw_file(raw_file) }; > + > + let ptr = match T::open(file, misc) { > Ok(ptr) => ptr, > Err(err) => return err.to_errno(), > }; > > - // SAFETY: The open call of a file owns the private data. > - unsafe { (*file).private_data = ptr.into_foreign().cast_mut() }; > + // This overwrites the private data with the value specified by the user, changing the type of > + // this file's private data. All future accesses to the private data is performed by other > + // fops_* methods in this file, which all correctly cast the private data to the new type. > + // > + // SAFETY: The open call of a file can access the private data. > + unsafe { (*raw_file).private_data = ptr.into_foreign().cast_mut() }; > > 0 > } > > -- > 2.47.1.613.gc27f4b7a9f-goog >
On Wed, 11 Dec 2024, Lee Jones wrote: > On Tue, 10 Dec 2024, Alice Ryhl wrote: > > > Providing access to the underlying `struct miscdevice` is useful for > > various reasons. For example, this allows you access the miscdevice's > > internal `struct device` for use with the `dev_*` printing macros. > > > > Note that since the underlying `struct miscdevice` could get freed at > > any point after the fops->open() call (if misc_deregister is called), > > only the open call is given access to it. To use `dev_*` printing macros > > from other fops hooks, take a refcount on `miscdevice->this_device` to > > keep it alive. See the linked thread for further discussion on the > > lifetime of `struct miscdevice`. > > > > Link: https://lore.kernel.org/r/2024120951-botanist-exhale-4845@gregkh > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > --- > > rust/kernel/miscdevice.rs | 30 ++++++++++++++++++++++-------- > > 1 file changed, 22 insertions(+), 8 deletions(-) > > Reviewed-by: Lee Jones <lee@kernel.org> Tested-by: Lee Jones <lee@kernel.org>
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs index 0cb79676c139..75a9d26c8001 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -97,14 +97,14 @@ fn drop(self: Pin<&mut Self>) { /// Trait implemented by the private data of an open misc device. #[vtable] -pub trait MiscDevice { +pub trait MiscDevice: Sized { /// What kind of pointer should `Self` be wrapped in. type Ptr: ForeignOwnable + Send + Sync; /// Called when the misc device is opened. /// /// The returned pointer will be stored as the private data for the file. - fn open(_file: &File) -> Result<Self::Ptr>; + fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>; /// Called when the misc device is released. fn release(device: Self::Ptr, _file: &File) { @@ -182,24 +182,38 @@ impl<T: MiscDevice> VtableHelper<T> { /// The file must be associated with a `MiscDeviceRegistration<T>`. unsafe extern "C" fn fops_open<T: MiscDevice>( inode: *mut bindings::inode, - file: *mut bindings::file, + raw_file: *mut bindings::file, ) -> c_int { // SAFETY: The pointers are valid and for a file being opened. - let ret = unsafe { bindings::generic_file_open(inode, file) }; + let ret = unsafe { bindings::generic_file_open(inode, raw_file) }; if ret != 0 { return ret; } + // SAFETY: The open call of a file can access the private data. + let misc_ptr = unsafe { (*raw_file).private_data }; + + // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the + // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()` + // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`. + let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() }; + // SAFETY: - // * The file is valid for the duration of this call. + // * This underlying file is valid for (much longer than) the duration of `T::open`. // * There is no active fdget_pos region on the file on this thread. - let ptr = match T::open(unsafe { File::from_raw_file(file) }) { + let file = unsafe { File::from_raw_file(raw_file) }; + + let ptr = match T::open(file, misc) { Ok(ptr) => ptr, Err(err) => return err.to_errno(), }; - // SAFETY: The open call of a file owns the private data. - unsafe { (*file).private_data = ptr.into_foreign().cast_mut() }; + // This overwrites the private data with the value specified by the user, changing the type of + // this file's private data. All future accesses to the private data is performed by other + // fops_* methods in this file, which all correctly cast the private data to the new type. + // + // SAFETY: The open call of a file can access the private data. + unsafe { (*raw_file).private_data = ptr.into_foreign().cast_mut() }; 0 }
Providing access to the underlying `struct miscdevice` is useful for various reasons. For example, this allows you access the miscdevice's internal `struct device` for use with the `dev_*` printing macros. Note that since the underlying `struct miscdevice` could get freed at any point after the fops->open() call (if misc_deregister is called), only the open call is given access to it. To use `dev_*` printing macros from other fops hooks, take a refcount on `miscdevice->this_device` to keep it alive. See the linked thread for further discussion on the lifetime of `struct miscdevice`. Link: https://lore.kernel.org/r/2024120951-botanist-exhale-4845@gregkh Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- rust/kernel/miscdevice.rs | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)