Messages in this thread | | | Date | Wed, 25 Jun 2008 19:12:09 -0700 | From | Joel Becker <> | Subject | Re: configfs: Q: item leak in a failing configfs_attach_group()? |
| |
On Wed, Jun 25, 2008 at 11:55:27AM +0200, Louis Rilling wrote: > Back to the two solutions that I've suggested (copy-pasted below), which one > would you prefer?
God, this is all ugly. You've found so many ugly cases :-(
> If I'm right, two kinds of solutions for issue 1 (new item created while > attaching a default group hierarchy): > i/ tag new directories with CONFIGFS_USET_NEW before calling d_instantiate, and > validate the whole group+default groups hierarchy in a second pass by clearing > CONFIGFS_USET_NEW
I think this is the right way. We can't d_instantiate() later, because lower callers use dentry->d_inode, and trying to work around that would be even uglier! But can't we just propagate CONFIGFS_USET_MKDIR? That's what's happening actually. Just set it down in the paths. Then, change like so:
if (group) ret = configfs_attach_group(parent_item, item, dentry); else ret = configfs_attach_item(parent_item, item, dentry); spin_lock(&configfs_dirent_lock); sd->s_type &= ~CONFIGFS_USET_IN_MKDIR; + if (!ret) + configfs_clear_mkdir_flag(dentry); spin_unlock(&configfs_dirent_lock);
Right?
> For issue 2/ (detach_item() called without locking the detached item's inode), > locking the inode before calling detach_item() (as is done from > configfs_rmdir()), plus a solution for 1/ should be sufficient.
Make sure you lock/unlock in the right place (mirror the teardown path).
Joel
--
A good programming language should have features that make the kind of people who use the phrase "software engineering" shake their heads disapprovingly. - Paul Graham
Joel Becker Principal Software Developer Oracle E-mail: joel.becker@oracle.com Phone: (650) 506-8127
| |