debugfs and module unloading

debugfs and module unloading

[Dave P Martin]

Hi there,

I have a quick debugfs question:

How can I protect against a module being unloaded while debugfs files it
provides are open?

I can do something like the following:

static int my_debugfs_file_open(struct inode *, struct file *)
{
	if (!try_module_get())
		return -EIO;

	/* ... */
}

static int my_debugfs_file_release(struct inode *, struct file *)
{
	/* ... */

	module_put();

	return 0;
}

static const struct file_operations my_debugfs_fops = {
	/* ... */

	.open = my_debugfs_file_open,
	.release = my_debugfs_file_release,

	/* ... */
};

static struct device_driver my_driver {
	/* ... */

	.owner = THIS_MODULE;

	/* ... */
};

static int my_module_init(void)
{
	driver_register(&my_driver);
	debugfs_create_file(..., &my_debugfs_fops);
}

static void my_module_exit(void)
{
	debugfs_remove_recusrive(...);
	driver_unregister(&my_driver);
}


... but it doesn't quite work.  debugfs_remove_recursive() prevents any
new file handles being opened, but files already open remain open -- so
it's still possible for the module text to get unloaded between the
module refcount being decreased inside module_put() and the time
my_debugfs_file_release() returns.

The scenario to consider is when a request to unload the module races
with closure of the last debugfs file.


The only obvious way I can see to solve this without changing the debugfs
code is to make the module impossible to unload by calling __module_get()
during initialisation, before any debugfs file is created.


A similar dependency problem exists when a pointer to some device
instance data is passed to debugfs_create_file().  For pluggable
devices, the device might go away at any time.  I hoped this could
be solved by calling get_device() ... put_device() in the debugfs file
open and release methods, but I found that a device instance can
get removed, and the module unloaded, even though the struct device
refcount is not zero.


Am I missing something?

Cheers
---Dave

Re: debugfs and module unloading

[Greg Kroah-Hartman]

On Wed, May 21, 2014 at 05:48:28PM +0100, Dave P Martin wrote:
> Hi there,
> 
> I have a quick debugfs question:
> 
> How can I protect against a module being unloaded while debugfs files it
> provides are open?
> 
> I can do something like the following:
> 
> static int my_debugfs_file_open(struct inode *, struct file *)
> {
> 	if (!try_module_get())
> 		return -EIO;
> 
> 	/* ... */
> }
> 
> static int my_debugfs_file_release(struct inode *, struct file *)
> {
> 	/* ... */
> 
> 	module_put();
> 
> 	return 0;
> }
> 
> static const struct file_operations my_debugfs_fops = {
> 	/* ... */
> 
> 	.open = my_debugfs_file_open,
> 	.release = my_debugfs_file_release,
> 
> 	/* ... */
> };
> 
> static struct device_driver my_driver {
> 	/* ... */
> 
> 	.owner = THIS_MODULE;
> 
> 	/* ... */
> };
> 
> static int my_module_init(void)
> {
> 	driver_register(&my_driver);
> 	debugfs_create_file(..., &my_debugfs_fops);
> }
> 
> static void my_module_exit(void)
> {
> 	debugfs_remove_recusrive(...);
> 	driver_unregister(&my_driver);
> }
> 
> 
> ... but it doesn't quite work.  debugfs_remove_recursive() prevents any
> new file handles being opened, but files already open remain open -- so
> it's still possible for the module text to get unloaded between the
> module refcount being decreased inside module_put() and the time
> my_debugfs_file_release() returns.
> 
> The scenario to consider is when a request to unload the module races
> with closure of the last debugfs file.
> 
> 
> The only obvious way I can see to solve this without changing the debugfs
> code is to make the module impossible to unload by calling __module_get()
> during initialisation, before any debugfs file is created.
> 
> 
> A similar dependency problem exists when a pointer to some device
> instance data is passed to debugfs_create_file().  For pluggable
> devices, the device might go away at any time.  I hoped this could
> be solved by calling get_device() ... put_device() in the debugfs file
> open and release methods, but I found that a device instance can
> get removed, and the module unloaded, even though the struct device
> refcount is not zero.
> 
> 
> Am I missing something?

Nope, you are not, it's a known issue.  Al Viro is doing some core VFS
work to help mitigate this, and I am working on converting debugfs to
use kernfs which should also help resolve this problem.

Don't unload modules automatically and you should be fine :)

thanks,

greg k-h

Re: debugfs and module unloading

[Dave Martin]

On Wed, May 21, 2014 at 11:01:14PM +0100, Greg Kroah-Hartman wrote:
> On Wed, May 21, 2014 at 05:48:28PM +0100, Dave P Martin wrote:
> > Hi there,
> > 
> > I have a quick debugfs question:
> > 
> > How can I protect against a module being unloaded while debugfs files it
> > provides are open?
> > 
> > I can do something like the following:

[...]

> > ... but it doesn't quite work.  debugfs_remove_recursive() prevents any
> > new file handles being opened, but files already open remain open -- so
> > it's still possible for the module text to get unloaded between the
> > module refcount being decreased inside module_put() and the time
> > my_debugfs_file_release() returns.
> > 
> > The scenario to consider is when a request to unload the module races
> > with closure of the last debugfs file.
> > 
> > 
> > The only obvious way I can see to solve this without changing the debugfs
> > code is to make the module impossible to unload by calling __module_get()
> > during initialisation, before any debugfs file is created.
> > 
> > 
> > A similar dependency problem exists when a pointer to some device
> > instance data is passed to debugfs_create_file().  For pluggable
> > devices, the device might go away at any time.  I hoped this could
> > be solved by calling get_device() ... put_device() in the debugfs file
> > open and release methods, but I found that a device instance can
> > get removed, and the module unloaded, even though the struct device
> > refcount is not zero.
> > 
> > 
> > Am I missing something?
> 
> Nope, you are not, it's a known issue.  Al Viro is doing some core VFS
> work to help mitigate this, and I am working on converting debugfs to
> use kernfs which should also help resolve this problem.
> 
> Don't unload modules automatically and you should be fine :)

OK, I will stop attempting half-baked workarounds of my own and keep
an eye out for changes in VFS.

Unloading the module certainly speeds up development work, but users
of it are unlikely to care...

Thanks
---Dave