Message-ID: <2024120954-boring-skeptic-ad16@gregkh>
Date: Mon, 9 Dec 2024 12:09:55 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Arnd Bergmann <arnd@...db.de>, Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	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>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Trevor Gross <tmgross@...ch.edu>, Lee Jones <lee@...nel.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] rust: miscdevice: access the `struct miscdevice`
 from fops->open()

On Mon, Dec 09, 2024 at 11:50:57AM +0100, Alice Ryhl wrote:
> On Mon, Dec 9, 2024 at 9:48 AM Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> wrote:
> >
> > On Mon, Dec 09, 2024 at 07:27:47AM +0000, 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, only the open call is given
> > > access to it. To print from other calls, they should take a refcount on
> > > the device to keep it alive.
> >
> > The lifespan of the miscdevice is at least from open until close, so
> > it's safe for at least then (i.e. read/write/ioctl/etc.)
> 
> How is that enforced? What happens if I call misc_deregister while
> there are open fds?

You shouldn't be able to do that as the code that would be calling
misc_deregister() (i.e. in a module unload path) would not work because
the module reference count is incremented at this point in time due to
the file operation module reference.

Wait, we are plumbing in the module owner logic here, right?  That
should be in the file operations structure.

Yeah, it's a horrid hack, and one day we will put "real" revoke logic in
here to detach the misc device from the file operations if this were to
happen.  It's a very very common anti-pattern that many subsystems have
that is a bug that we all have been talking about for a very very long
time.  Wolfram even has a plan for how to fix it all up (see his Japan
LinuxCon talk from 2 years ago), but I don't think anyone is doing the
work on it :(

The media and drm layers have internal hacks/work-arounds to try to
handle this issue, but luckily for us, the odds of a misc device being
dynamically removed from the system is pretty low.

Once / if ever, we get the revoke type logic implemented, then we can
apply that to the misc device code and follow it through to the rust
side if needed.

> > > Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> > > ---
> > >  rust/kernel/miscdevice.rs | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > > index 0cb79676c139..c5af1d5ec4be 100644
> > > --- a/rust/kernel/miscdevice.rs
> > > +++ b/rust/kernel/miscdevice.rs
> > > @@ -104,7 +104,7 @@ pub trait MiscDevice {
> > >      /// 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) {
> > > @@ -190,14 +190,27 @@ impl<T: MiscDevice> VtableHelper<T> {
> > >          return ret;
> > >      }
> > >
> > > +    // SAFETY: The opwn call of a file can access the private data.
> >
> > s/opwn/open/ :)
> >
> > > +    let misc_ptr = unsafe { (*file).private_data };
> >
> > Blank line here?
> >
> > > +    // 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`.
> >
> > Aren't we wrapping comment lines at 80 columns still?  I can't remember
> > anymore...
> 
> Not sure what the rules are, but I don't think Rust comments are being
> wrapped at 80.

Ok, that's fine, I didn't remember either.

> > > +    let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
> > > +
> > >      // SAFETY:
> > > -    // * The file is valid for the duration of this call.
> > > +    // * The file is valid for the duration of the `T::open` call.
> >
> > It's valid for the lifespan between open/release.
> >
> > >      // * 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(file) };
> > > +
> > > +    let ptr = match T::open(file, misc) {
> > >          Ok(ptr) => ptr,
> > >          Err(err) => return err.to_errno(),
> > >      };
> > >
> > > +    // This overwrites the private data from above. It makes sense to not hold on to the misc
> > > +    // pointer since the `struct miscdevice` can get unregistered as soon as we return from this
> > > +    // call, so the misc pointer might be dangling on future file operations.
> > > +    //
> >
> > Wait, what are we overwriting this here with?  Now private data points
> > to the misc device when before it was the file structure.  No other code
> > needed to be changed because of that?  Can't we enforce this pointer
> > type somewhere so that any casts in any read/write/ioctl also "knows" it
> > has the right type?  This feels "dangerous" to me.
> 
> Ultimately, when interfacing with C code using void pointers, Rust is
> going to need a pointer cast somewhere to assert what the type is.
> With the current design, that place is the fops_* functions. We need
> to get the pointer casts right there, but anywhere else the types are
> enforced.

So where else is this type enforced?  A read/write/ioctl call also wants
this pointer, or is it up to the open call to stash it somewhere that
those calls can get to it?  It's hanging off of the file pointer now:

> > >      // SAFETY: The open call of a file owns the private data.
> > >      unsafe { (*file).private_data = ptr.into_foreign().cast_mut() };

So any place that casts back from that structure needs to get this
correct.  Is that up to each individual misc device driver to do it (to
be fair, that's what the C drivers do, just wanting to be sure here.)

Ah, wait, I get it, you are just storing the "raw" pointer to the rust
implementation of the structure, NOT the C "raw" pointer like it
currently is today.  You are making this all match up with the existing
code.

Sorry for the noise, this all makes sense to me now, I didn't have
enough coffee for that first code review.

> >
> > Is this SAFETY comment still correct?
> 
> Well, it could probably be worded better at least. The point is that
> nobody else is going to touch this field and we can do what we want
> with it.

True, ok, that's fine.

Care to respin this with at least the typo fixed?

thanks,

greg k-h

Powered by blists - more mailing lists