Re: [PATCH v2 2/2] rust: miscdevice: access the `struct miscdevice` from fops->open()

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Dec 09, 2024 at 12:38:32PM +0100, Alice Ryhl wrote:
> On Mon, Dec 9, 2024 at 12:10 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > 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@linuxfoundation.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.
> 
> Oh .. so misc_deregister must only be called when the module is being unloaded?

Traditionally yes, that's when it is called.  Do you see it happening in
any other place in the kernel today?

> > Wait, we are plumbing in the module owner logic here, right?  That
> > should be in the file operations structure.
> 
> Right ... it's missing but I will add it.

Thanks!

> > 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.
> 
> If dynamically deregistering is not safe, then we need to change the
> Rust abstractions to prevent it.

Dynamically deregistering is not unsafe, it's just that I don't think
you will physically ever have the misc_deregister() path called if a
file handle is open.  Same should be the case for rust code, it should
"just work" without any extra code to do so.

thanks,

greg k-h