lkml.org 
[lkml]   [2008]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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


\
 
 \ /
  Last update: 2008-06-26 04:15    [W:0.070 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site