debugfs_remove() vs. anything that is dynamic

debugfs_remove() vs. anything that is dynamic

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Consider the following trivial module:

--- %< ---
#include <linux/module.h>
#include <linux/debugfs.h>

static struct dentry *f;
static u32 tmp;

int __init mod_enter(void)
{
	f = debugfs_create_u32("tmp-test", 0666, NULL, &tmp);

	return 0;
}

void __exit mod_leave(void)
{
	debugfs_remove(f);
}

module_init(mod_enter);
module_exit(mod_leave);
MODULE_LICENSE("GPL");
--- >% ---

How do I make that safe?


FWIW, the problem is:

thread 1			thread 2
 fd = open("tmp-test")

 sleep(30);			rmmod test-module

 read(fd, buf, 100);

--> accesses now invalid memory because debugfs doesn't actually stop
you from accessing "&tmp" after debugfs_remove(). [yes, I actually
tested a variation of this where I dynamically allocated the 'tmp'
variable, I got the slab poison in my test program]


Personally, I tend to think this makes debugfs rather unusable in
modules and with anything that is dynamically allocated [1]. AFAICT
sysfs avoids this by having object lifetime imposed by sysfs, but
debugfs doesn't work that way.

What am I missing?

johannes

[1] which covers many many current users, it seems at least usbmon,
ohci/ehci/uhci-dbg, pktcdvd, fault injection code, blktrace and probably
more.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: debugfs_remove() vs. anything that is dynamic

[Greg KH]

On Fri, Apr 04, 2008 at 01:56:26AM +0200, Johannes Berg wrote:
> Consider the following trivial module:
> 
> --- %< ---
> #include <linux/module.h>
> #include <linux/debugfs.h>
> 
> static struct dentry *f;
> static u32 tmp;
> 
> int __init mod_enter(void)
> {
> 	f = debugfs_create_u32("tmp-test", 0666, NULL, &tmp);
> 
> 	return 0;
> }
> 
> void __exit mod_leave(void)
> {
> 	debugfs_remove(f);
> }
> 
> module_init(mod_enter);
> module_exit(mod_leave);
> MODULE_LICENSE("GPL");
> --- >% ---
> 
> How do I make that safe?
> 
> 
> FWIW, the problem is:
> 
> thread 1			thread 2
>  fd = open("tmp-test")
> 
>  sleep(30);			rmmod test-module
> 
>  read(fd, buf, 100);
> 
> --> accesses now invalid memory because debugfs doesn't actually stop
> you from accessing "&tmp" after debugfs_remove(). [yes, I actually
> tested a variation of this where I dynamically allocated the 'tmp'
> variable, I got the slab poison in my test program]
> 
> 
> Personally, I tend to think this makes debugfs rather unusable in
> modules and with anything that is dynamically allocated [1]. AFAICT
> sysfs avoids this by having object lifetime imposed by sysfs, but
> debugfs doesn't work that way.
> 
> What am I missing?

If you worry about this type of interaction, use debugfs_create_file,
which takes a fileops, and set your module owner in there so that the
reference count will not allow your module from being removed.

Also remember, you have to be root to unload modules, so if you are
doing that, and you have debugfs files open, you should know better :)

thanks,

greg k-h

Re: debugfs_remove() vs. anything that is dynamic

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]


> If you worry about this type of interaction, use debugfs_create_file,
> which takes a fileops, and set your module owner in there so that the
> reference count will not allow your module from being removed.
>
> Also remember, you have to be root to unload modules, so if you are
> doing that, and you have debugfs files open, you should know better :)

That really was just an example, we have per-wireless-device debugfs
code users can easily trigger a debugfs_remove() by unplugging a usb
netdevice for example. Also, that means that anything that is dynamic
would have the lifetime rules imposed by debugfs which is rather
awkward.

The current code for simple_attr_open copies the in i_private pointer:

attr->data = inode->i_private;

if, instead, it would keep a reference to that, like

attr->dataptr = &inode->i_private;

we could NULL out that pointer on debugfs_remove() and have
simple_attr_read() just return -ENOENT.


However, if nobody else is concerned about this, I'll just remove all
the wireless debugfs code instead, I just don't want to allow crashing
it that way.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: debugfs_remove() vs. anything that is dynamic

[Greg KH]

On Fri, Apr 04, 2008 at 05:41:24PM +0200, Johannes Berg wrote:
> 
> > If you worry about this type of interaction, use debugfs_create_file,
> > which takes a fileops, and set your module owner in there so that the
> > reference count will not allow your module from being removed.
> >
> > Also remember, you have to be root to unload modules, so if you are
> > doing that, and you have debugfs files open, you should know better :)
> 
> That really was just an example, we have per-wireless-device debugfs
> code users can easily trigger a debugfs_remove() by unplugging a usb
> netdevice for example. Also, that means that anything that is dynamic
> would have the lifetime rules imposed by debugfs which is rather
> awkward.

Then use the debugfs_create_file() function instead of the individual
variable ones if this is the issue.  You can easily wrap them up much
like the debugfs core does with the common wrappers to allow for the
proper module lifetime rules to be followed.

> The current code for simple_attr_open copies the in i_private pointer:
> 
> attr->data = inode->i_private;
> 
> if, instead, it would keep a reference to that, like
> 
> attr->dataptr = &???inode->i_private;
> 
> we could NULL out that pointer on debugfs_remove() and have
> simple_attr_read() just return -ENOENT.

Patches are always gladly accepted :)

> However, if nobody else is concerned about this, I'll just remove all
> the wireless debugfs code instead, I just don't want to allow crashing
> it that way.

I think you should be able to handle this in your debugfs calls, or if
needed, we can easily add a new parameter to them to handle the module
ownership if you are concerned about them.

thanks,

greg k-h

Re: debugfs_remove() vs. anything that is dynamic

[Johannes Berg]

On Fri, 2008-04-04 at 13:20 -0700, Greg KH wrote:

> Then use the debugfs_create_file() function instead of the individual
> variable ones if this is the issue.  You can easily wrap them up much
> like the debugfs core does with the common wrappers to allow for the
> proper module lifetime rules to be followed.

Yeah, I was actually looking into this whole thing because I thought
that mac80211's debugfs code is too large (at about a quarter of the
binary size of the whole thing), so I'm not keen on adding more code, I
was rather wondering if it would be possible to remove a lot of the code
in favour of using simple attributes :)

> > we could NULL out that pointer on debugfs_remove() and have
> > simple_attr_read() just return -ENOENT.
> 
> Patches are always gladly accepted :)
> 
> > However, if nobody else is concerned about this, I'll just remove all
> > the wireless debugfs code instead, I just don't want to allow crashing
> > it that way.
> 
> I think you should be able to handle this in your debugfs calls, or if
> needed, we can easily add a new parameter to them to handle the module
> ownership if you are concerned about them.

No, I'm not particularly concerned about module ownership, I'm more
concerned about any dynamic objects.

This patch seems to work. My system boots (so sysfs definitely works)
and my debugfs test case returns -ENOENT on the read, even if the file
was opened previously.

I think, however, it's not correct when you have a hard-link, and the
NULLing out of i_private must be done depending on (inode->i_nlink == 1)
instead of unconditionally. I will have to test that once I figure out
if (and if yes, how) you can even create inodes with nlink>1 with simple
attributes.

--- everything.orig/fs/libfs.c	2008-04-04 22:37:18.000000000 +0200
+++ everything/fs/libfs.c	2008-04-04 22:37:37.000000000 +0200
@@ -287,6 +287,7 @@ int simple_unlink(struct inode *dir, str
 {
 	struct inode *inode = dentry->d_inode;
 
+	inode->i_private = NULL;
 	inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 	drop_nlink(inode);
 	dput(dentry);
@@ -587,7 +588,7 @@ struct simple_attr {
 	int (*set)(void *, u64);
 	char get_buf[24];	/* enough to store a u64 and "\n\0" */
 	char set_buf[24];
-	void *data;
+	void **dataptr;
 	const char *fmt;	/* format for read operation */
 	struct mutex mutex;	/* protects access to these buffers */
 };
@@ -606,7 +607,7 @@ int simple_attr_open(struct inode *inode
 
 	attr->get = get;
 	attr->set = set;
-	attr->data = inode->i_private;
+	attr->dataptr = &inode->i_private;
 	attr->fmt = fmt;
 	mutex_init(&attr->mutex);
 
@@ -642,9 +643,17 @@ ssize_t simple_attr_read(struct file *fi
 		size = strlen(attr->get_buf);
 	} else {		/* first read */
 		u64 val;
-		ret = attr->get(attr->data, &val);
-		if (ret)
+		void *data;
+
+		data = *attr->dataptr;
+		if (!data) {
+			ret = -ENOENT;
 			goto out;
+		} else {
+			ret = attr->get(data, &val);
+			if (ret)
+				goto out;
+		}
 
 		size = scnprintf(attr->get_buf, sizeof(attr->get_buf),
 				 attr->fmt, (unsigned long long)val);
@@ -664,6 +673,7 @@ ssize_t simple_attr_write(struct file *f
 	u64 val;
 	size_t size;
 	ssize_t ret;
+	void *data;
 
 	attr = file->private_data;
 	if (!attr->set)
@@ -678,10 +688,15 @@ ssize_t simple_attr_write(struct file *f
 	if (copy_from_user(attr->set_buf, buf, size))
 		goto out;
 
-	ret = len; /* claim we got the whole input */
-	attr->set_buf[size] = '\0';
-	val = simple_strtol(attr->set_buf, NULL, 0);
-	attr->set(attr->data, val);
+	data = *attr->dataptr;
+	if (!data) {
+		ret = -ENOENT;
+	} else {
+		ret = len; /* claim we got the whole input */
+		attr->set_buf[size] = '\0';
+		val = simple_strtol(attr->set_buf, NULL, 0);
+		attr->set(data, val);
+	}
 out:
 	mutex_unlock(&attr->mutex);
 	return ret;

Re: debugfs_remove() vs. anything that is dynamic

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]


> I think, however, it's not correct when you have a hard-link, and the
> NULLing out of i_private must be done depending on (inode->i_nlink == 1)
> instead of unconditionally. I will have to test that once I figure out
> if (and if yes, how) you can even create inodes with nlink>1 with simple
> attributes.
> 
> --- everything.orig/fs/libfs.c	2008-04-04 22:37:18.000000000 +0200
> +++ everything/fs/libfs.c	2008-04-04 22:37:37.000000000 +0200
> @@ -287,6 +287,7 @@ int simple_unlink(struct inode *dir, str
>  {
>  	struct inode *inode = dentry->d_inode;
>  
> +	inode->i_private = NULL;

Hm, no, this is likely to conflict with other users of simple_unlink
that use i_private afterwards and don't expect it to have been NULL'ed
out. I haven't found a solution yet.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]