Message-ID: <20121128093754.GA674@soda.linbit>
Date:	Wed, 28 Nov 2012 10:37:54 +0100
From:	Lars Ellenberg <lars.ellenberg@...bit.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [RFC] [PATCH] fix infinite loop; increase robustness of
 debugfs_remove_recursive [attached test module]

On Tue, Nov 27, 2012 at 10:16:28PM -0500, Steven Rostedt wrote:
> On Fri, 2012-11-23 at 18:15 +0100, Lars Ellenberg wrote:
> > When toying around with debugfs, intentionally trying to break things,
> > I managed to get it into a reproducible endless loop when cleaning up.
> > 
> > debugfs_remove_recursive() completely ignores that entries found
> > on ->d_subdirs may already be d_delete()d, but not yet d_kill()ed.
> > 
> > In this case, the first two entries found have both been such dentries
> > (d_iname = ".", btw), while later in the list there was still a real,
> > not yet deleted entry.
> > 
> > That way, the "goto next_sibling;" did not catch this case,
> > the "break the infinit loop if we fail to remove something"
> > did not catch it either.
> > 
> > 
> > Disclaimer: I'm not a dentries and inodes guy...
> 
> I'm not a dentries or inodes guy either, so I wont comment on the actual
> merits of this patch.

Though you submitted the "next_sibling" code back in 2009, right?
That's why you ended up on Cc.
I suspect that even back then, the real problem was
"already dead but still linked" dentries, as below:

> > Still, I think the "is this child a non-empty subdir" check
> > was just wrong. This is my fix:
> > -	if (list_empty(&child->d_subdirs)) 
> > +	if (!simple_emty(child))
> 
> "simple_empty"

Of course. I mistyped both lines in the email body :-/
In the patch below it was correct:
-               if (!list_empty(&child->d_subdirs)) {
+               if (!simple_empty(child)) {

> > Also, always trying to __debugfs_remove() the first entry found from
> > parent->d_subdirs.next is wrong, we need to skip over any already
> > deleted children. I introduced the debugfs_find_next_positive_child()
> > helper for this.
> > 
> > If I understand it correctly, if we do it this way, it can never fail.
> > That is, as long as we can be sure that no new dentries will be created
> > while we are in debugfs_remove_recursive().
> > So the 
> > 		if (debugfs_positive(child)) {
> >  			/*
> >  			 * Avoid infinite loop if we fail to remove
> >  			 * one dentry.
> > is probably dead code now?
> > 
> > 
> > As an additional fix, to prevent an access after free and resulting Oops,
> > I serialize dereferencing of attr->get/set and attr->data with d_delete(),
> > using the file->f_dentry->d_parent->d_inode->i_mutex.
> > 
> > If this file->f_dentry meanwhile has been deleted, simple_attr_read()
> > and simple_attr_write() now returns -ESTALE. (Any other error code preferences?)
> > 
> > 
> > With this patch, I was able to
> > cd /sys/debugfs/test-module/some/dir
> > exec 7< some-file
> > rmmod test-module
> > cat <&7
> 
> I saw this and thought "hmm, I wonder if the trace_events have issues,
> as they create debugfs directories and files via modules too". I went
> and tried to reproduce but couldn't get passed the rmmod, as the module
> count gets incremented for any open files that the module creates.

Is that so.

> I take it that you didn't add that feature to your test module.

If you use the "debugfs_create_u32" (and similar "_<simple attribute>")
wrappers, you don't get a chance to do so from the attribute callbacks,
all callbacks are predefined, you just submit a pointer.

You mean something like this, or am I missing something?
  __module_get();
  debugfs_create_u32();
And later
  debugfs_remove_recursive();
  module_put();

You still have the race between someone opening some attribute file,
you removing the attribute and releasing your refcount, then the other
guy dereferencing that pointer in the read() call.

I think you can only deal with that race,
if you provide your own file_operations for all your attributes.

I'm about to add some such debugfs entries for our driver, and would
like to avoid re-implementing all of the simple_attribute fops.

> > without any infinite loops, hangs, oopses or other problems,
> > and as expected get an ESTALE for the cat.
> > 
> > Without the patch, I'll get either an infinite loop and rmmod never
> > terminates, or cat oopses.
> > 
> > 
> > If you think this is correct, please comment on the FIXME
> > below, and help me write a nice commit message.
> > 
> > If you think this is still wrong or even makes things worse,
> > please help me with a proper fix ;-)
> > 
> > 
> > Patch is against upstream as of yesterday,
> > but looks like it still applies way back into 2009, 2.6.3x,
> > so if it is correct, it may even qualify for the stable branches?
> > 
> 
> Now, is there any current user of debugfs that is susceptible for this
> bug?

I'm unsure. Grepping for debugfs_create_u32, I suspect that some of the
crypto or wifi modules may be affected.

> I'm not saying that these issues shouldn't be fixed. But I'm also
> concerned about exploits and other things that just a root user may
> accidentally cause harm.

I'm concerned about monitoring/statistic gathering stuff (running as any
user) may hold some debugfs file open,
and race with removal of dynamic debugfs entries.

> If there's current problem then maybe this
> isn't needed for stable. But should probably be fixed for the future.

I attach my "hello-debugfs" module.
# make -C /lib/modules/`uname -r`/build M=$PWD modules

Test case1:
	cd /sys/kernel/debug/hello-debugfs/dir1/dir3/
	rmmod
will unload the module, but give up on removing
/sys/kernel/debug/hello-debugfs/dir1
You won't be able to insmod a second time.

Test case 2:
	cd /sys/kernel/debug/hello-debugfs/dir1/dir3/
	exec 7< file7
	rmmod
	cat <&7
As described above, depending on I don't know what exactly,
the outcome was either some infinite loop, or cat will oops/panic.

Note that all but rmmod may be done as normal user,
e.g. some monitoring or statistics gathering tool,
and the rmmod is just a placeholder for
"any event that triggers removal of dynamic debugfs entries".

	Lars


View attachment "hello-debugfs.c" of type "text/x-csrc" (1732 bytes)

View attachment "Kbuild" of type "text/plain" (63 bytes)

Powered by blists - more mailing lists