configfs: Q: item leak in a failing configfs_attach_group()?

configfs: Q: item leak in a failing configfs_attach_group()?

[Louis Rilling]

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

Hi,

I'd like an opinion on the following scenario:

process 1: 					process 2:
configfs_mkdir("A")
  attach_group("A")
    attach_item("A")
      d_instantiate("A")
    populate_groups("A")
      mutex_lock("A")
      attach_group("A/B")
        attach_item("A")
          d_instantiate("A/B")
						mkdir("A/B/C")
						  do_path_lookup("A/B/C", LOOKUP_PARENT)
						    ok
						  lookup_create("A/B/C")
						    mutex_lock("A/B")
						    ok
						  configfs_mkdir("A/B/C")
						    ok
      attach_group("A/C")
        attach_item("A/C")
          d_instantiate("A/C")
        populate_groups("A/C")
          mutex_lock("A/C")
          attach_group("A/C/D")
            attach_item("A/C/D")
              failure
          mutex_unlock("A/C")
          detach_groups("A/C")
            nothing to do
						mkdir("A/C/E")
						  do_path_lookup("A/C/E", LOOKUP_PARENT)
						    ok
						  lookup_create("A/C/E")
						    mutex_lock("A/C")
						    ok
						  configfs_mkdir("A/C/E")
						    ok
        detach_item("A/C")
        d_delete("A/C")
      mutex_unlock("A")
      detach_groups("A")
        mutex_lock("A/B")
        detach_group("A/B")
	  detach_groups("A/B")
	    nothing since no _default_ group
          detach_item("A/B")
        mutex_unlock("A/B")
        d_delete("A/B")
    detach_item("A")
    d_delete("A")

Two bugs (if the scenario is possible):

1/ "A/B/C" and "A/C/E" are created, but never removed while their parent are
removed in the end.

2/ "A" and "A/C" inodes are not locked while detach_item() is called on them,
   which may probably confuse VFS.

Is there something that prevents such scenario? I'd say that once dentries
are instantiated, the dcache does not need to lock their inode to traverse them,
so the scenario is possible.

Where am I wrong?

If I'm right, two kinds of solutions for issue 1:
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

ii/ do not call d_instantiate() immediately in configfs_create() if called from
configfs_create_dir(), and d_instantitate() the group+default groups hierarchy
in a second pass. Problem: is it correct to add children to a dentry which is
not yet instantiated?

For issue 2/, locking the inode before calling detach_item() (as is done from
configfs_rmdir()), plus a solution for 1/ should be sufficient.

Thanks for your explanations.

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: configfs: Q: item leak in a failing configfs_attach_group()?

[Joel Becker]

On Tue, Jun 24, 2008 at 04:16:49PM +0200, Louis Rilling wrote:
> Hi,
> 
> I'd like an opinion on the following scenario:
> 
> process 1: 					process 2:
> configfs_mkdir("A")
>   attach_group("A")
>     attach_item("A")
>       d_instantiate("A")
>     populate_groups("A")
>       mutex_lock("A")
>       attach_group("A/B")
>         attach_item("A")
>           d_instantiate("A/B")
> 						mkdir("A/B/C")
> 						  do_path_lookup("A/B/C", LOOKUP_PARENT)

					This has to sleep until
					configfs_mkdir("A") finishes.
					It's waiting on A->d_parent's
					i_mutex, which is held by
					sys_mkdirat().

Joel

-- 

"Sometimes one pays most for the things one gets for nothing."
        - Albert Einstein

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: configfs: Q: item leak in a failing configfs_attach_group()?

[Louis Rilling]

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

On Tue, Jun 24, 2008 at 10:10:51AM -0700, Joel Becker wrote:
> On Tue, Jun 24, 2008 at 04:16:49PM +0200, Louis Rilling wrote:
> > Hi,
> > 
> > I'd like an opinion on the following scenario:
> > 
> > process 1: 					process 2:
> > configfs_mkdir("A")
> >   attach_group("A")
> >     attach_item("A")
> >       d_instantiate("A")
> >     populate_groups("A")
> >       mutex_lock("A")
> >       attach_group("A/B")
> >         attach_item("A")
> >           d_instantiate("A/B")
> > 						mkdir("A/B/C")
> > 						  do_path_lookup("A/B/C", LOOKUP_PARENT)
> 
> 					This has to sleep until
> 					configfs_mkdir("A") finishes.
> 					It's waiting on A->d_parent's
> 					i_mutex, which is held by
> 					sys_mkdirat().

Can you be more precise? I don't see where do_path_lookup() locks an inode
outside of real_lookup(), and what I understand is that real_lookup() is neither
called for "A", nor called for "A/B" because __d_lookup() will find positive
dentries (they were d_instantiated). Since do_path_lookup() is called with
LOOKUP_PARENT, it stops here. Then lookup_create() locks "A/B"'s inode, without
waiting, and calls lookup_hash("A/B", "C"), which ends calling
configfs_lookup("A/B", "C") because "A/B" has no child "C" in the dcache.

What am I missing?

Thanks,

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: configfs: Q: item leak in a failing configfs_attach_group()?

[Joel Becker]

On Tue, Jun 24, 2008 at 08:04:56PM +0200, Louis Rilling wrote:
> On Tue, Jun 24, 2008 at 10:10:51AM -0700, Joel Becker wrote:
> > On Tue, Jun 24, 2008 at 04:16:49PM +0200, Louis Rilling wrote:
> > > Hi,
> > > 
> > > I'd like an opinion on the following scenario:
> > > 
> > > process 1: 					process 2:
> > > configfs_mkdir("A")
> > >   attach_group("A")
> > >     attach_item("A")
> > >       d_instantiate("A")
> > >     populate_groups("A")
> > >       mutex_lock("A")
> > >       attach_group("A/B")
> > >         attach_item("A")
> > >           d_instantiate("A/B")
> > > 						mkdir("A/B/C")
> > > 						  do_path_lookup("A/B/C", LOOKUP_PARENT)
> > 
> > 					This has to sleep until
> > 					configfs_mkdir("A") finishes.
> > 					It's waiting on A->d_parent's
> > 					i_mutex, which is held by
> > 					sys_mkdirat().
> 
> Can you be more precise? I don't see where do_path_lookup() locks an inode

	It doesn't.  It's in lookup_create(), which takes the mutex on the
parent of 'A'.  Note that the end of sys_mkdirat() explicitly drops that
mutex - it couldn't do so if it hadn't been taken :-)

Joel

-- 

"You cannot bring about prosperity by discouraging thrift. You cannot
 strengthen the weak by weakening the strong. You cannot help the wage
 earner by pulling down the wage payer. You cannot further the
 brotherhood of man by encouraging class hatred. You cannot help the
 poor by destroying the rich. You cannot build character and courage by
 taking away a man's initiative and independence. You cannot help men
 permanently by doing for them what they could and should do for
 themselves."
	- Abraham Lincoln 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: configfs: Q: item leak in a failing configfs_attach_group()?

[Louis Rilling]

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

On Tue, Jun 24, 2008 at 02:34:39PM -0700, Joel Becker wrote:
> On Tue, Jun 24, 2008 at 08:04:56PM +0200, Louis Rilling wrote:
> > On Tue, Jun 24, 2008 at 10:10:51AM -0700, Joel Becker wrote:
> > > On Tue, Jun 24, 2008 at 04:16:49PM +0200, Louis Rilling wrote:
> > > > Hi,
> > > > 
> > > > I'd like an opinion on the following scenario:
> > > > 
> > > > process 1: 					process 2:
> > > > configfs_mkdir("A")
> > > >   attach_group("A")
> > > >     attach_item("A")
> > > >       d_instantiate("A")
> > > >     populate_groups("A")
> > > >       mutex_lock("A")
> > > >       attach_group("A/B")
> > > >         attach_item("A")
> > > >           d_instantiate("A/B")
> > > > 						mkdir("A/B/C")
> > > > 						  do_path_lookup("A/B/C", LOOKUP_PARENT)
> > > 
> > > 					This has to sleep until
> > > 					configfs_mkdir("A") finishes.
> > > 					It's waiting on A->d_parent's
> > > 					i_mutex, which is held by
> > > 					sys_mkdirat().
> > 
> > Can you be more precise? I don't see where do_path_lookup() locks an inode
> 
> 	It doesn't.  It's in lookup_create(), which takes the mutex on the
> parent of 'A'.  Note that the end of sys_mkdirat() explicitly drops that
> mutex - it couldn't do so if it hadn't been taken :-)

So, my scenario is realistic. Process 2 only locks "B"'s inode in
lookup_create() ("B" is the parent of the new directory "C"), and never has to
lock "A" or "A"'s parent. IOW, process 2 does not have to wait on any i_mutex
locked by process 1.

Back to the two solutions that I've suggested (copy-pasted below), which one
would you prefer?

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

ii/ do not call d_instantiate() immediately in configfs_create() if called from
configfs_create_dir(), and d_instantitate() the group+default groups hierarchy
in a second pass. Problem: is it correct to add children to a dentry which is
not yet instantiated?

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.

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: configfs: Q: item leak in a failing configfs_attach_group()?

[Joel Becker]

On Wed, Jun 25, 2008 at 11:55:27AM +0200, Louis Rilling wrote:
> On Tue, Jun 24, 2008 at 02:34:39PM -0700, Joel Becker wrote:
> > On Tue, Jun 24, 2008 at 08:04:56PM +0200, Louis Rilling wrote:
> > > On Tue, Jun 24, 2008 at 10:10:51AM -0700, Joel Becker wrote:
> > > > On Tue, Jun 24, 2008 at 04:16:49PM +0200, Louis Rilling wrote:
> > > > > Hi,
> > > > > 
> > > > > I'd like an opinion on the following scenario:
> > > > > 
> > > > > process 1: 					process 2:
> > > > > configfs_mkdir("A")
> > > > >   attach_group("A")
> > > > >     attach_item("A")
> > > > >       d_instantiate("A")
> > > > >     populate_groups("A")
> > > > >       mutex_lock("A")
> > > > >       attach_group("A/B")
> > > > >         attach_item("A")
> > > > >           d_instantiate("A/B")
> > > > > 						mkdir("A/B/C")
> > > > > 						  do_path_lookup("A/B/C", LOOKUP_PARENT)
> > > > 
> > > > 					This has to sleep until
> > > > 					configfs_mkdir("A") finishes.
> > > > 					It's waiting on A->d_parent's
> > > > 					i_mutex, which is held by
> > > > 					sys_mkdirat().
> > > 
> > > Can you be more precise? I don't see where do_path_lookup() locks an inode
> > 
> > 	It doesn't.  It's in lookup_create(), which takes the mutex on the
> > parent of 'A'.  Note that the end of sys_mkdirat() explicitly drops that
> > mutex - it couldn't do so if it hadn't been taken :-)
> 
> So, my scenario is realistic. Process 2 only locks "B"'s inode in
> lookup_create() ("B" is the parent of the new directory "C"), and never has to
> lock "A" or "A"'s parent. IOW, process 2 does not have to wait on any i_mutex
> locked by process 1.

	Um, 'A' hasn't appeared yet.  I don't see how it looks up 'A'
until we're done.

Joel

-- 

"When ideas fail, words come in very handy." 
         - Goethe

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: configfs: Q: item leak in a failing configfs_attach_group()?

[Joel Becker]

On Wed, Jun 25, 2008 at 01:20:58PM -0700, Joel Becker wrote:
> 	Um, 'A' hasn't appeared yet.  I don't see how it looks up 'A'
> until we're done.

	Oh, I think you mean cached_lookup isn't blocked by i_mutex.

Joel

-- 

Life's Little Instruction Book #497

	"Go down swinging."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: configfs: Q: item leak in a failing configfs_attach_group()?

[Joel Becker]

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