[PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[Maneesh Soni]

Hi,

sysfs_remove_dir modifies d_subdirs list which results in inconsistency
when there is concurrent dcache_readdir is going on. I think there
is no need for sysfs_remove_dir to modify d_subdirs list and removal
of dentries from d_child list is taken care in the final dput().

The folloing patch fixes this race. The patch is tested by running these two
loops simlutaneously on a SMP box.

#while true; do tree /sys/class/net > /dev/null; done

#while true; do insmod ./dummy.o; rmmod dummy.o; done


o This patch fixes sysfs_remove_dir race with dcache_readdir. There is
  no need for sysfs_remove_dir to modify the d_subdirs list for the directory
  being deleted as it is taken care in the final dput. Modifying this list
  results in inconsistent d_subdirs list and causes infinite loop in 
  concurrently occuring dcache_readdir.


 fs/sysfs/dir.c |    4 +---
 1 files changed, 1 insertion(+), 3 deletions(-)

diff -puN fs/sysfs/dir.c~sysfs_remove_dir-race-fix fs/sysfs/dir.c
--- linux-2.6.0-test9-bk24/fs/sysfs/dir.c~sysfs_remove_dir-race-fix	2003-11-20 10:36:13.000000000 +0530
+++ linux-2.6.0-test9-bk24-maneesh/fs/sysfs/dir.c	2003-11-20 10:38:32.000000000 +0530
@@ -122,8 +122,8 @@ void sysfs_remove_dir(struct kobject * k
 	node = dentry->d_subdirs.next;
 	while (node != &dentry->d_subdirs) {
 		struct dentry * d = list_entry(node,struct dentry,d_child);
-		list_del_init(node);
 
+		node = node->next;
 		pr_debug(" o %s (%d): ",d->d_name.name,atomic_read(&d->d_count));
 		if (d->d_inode) {
 			d = dget_locked(d);
@@ -139,9 +139,7 @@ void sysfs_remove_dir(struct kobject * k
 			spin_lock(&dcache_lock);
 		}
 		pr_debug(" done\n");
-		node = dentry->d_subdirs.next;
 	}
-	list_del_init(&dentry->d_child);
 	spin_unlock(&dcache_lock);
 	up(&dentry->d_inode->i_sem);
 

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Re: [PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[viro]

On Thu, Nov 20, 2003 at 11:17:07AM +0530, Maneesh Soni wrote:
> Hi,
> 
> sysfs_remove_dir modifies d_subdirs list which results in inconsistency
> when there is concurrent dcache_readdir is going on. I think there
> is no need for sysfs_remove_dir to modify d_subdirs list and removal
> of dentries from d_child list is taken care in the final dput().

WTF?

All instances of ->readdir() are protected by ->i_sem on parent.  So
are all operations in sysfs_remove_dir().

Have you actually observed any race there?

Re: [PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[Maneesh Soni]

On Thu, Nov 20, 2003 at 05:49:57AM +0000, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Thu, Nov 20, 2003 at 11:17:07AM +0530, Maneesh Soni wrote:
> > Hi,
> > 
> > sysfs_remove_dir modifies d_subdirs list which results in inconsistency
> > when there is concurrent dcache_readdir is going on. I think there
> > is no need for sysfs_remove_dir to modify d_subdirs list and removal
> > of dentries from d_child list is taken care in the final dput().
> 
> WTF?
> 
> All instances of ->readdir() are protected by ->i_sem on parent.  So
> are all operations in sysfs_remove_dir().
> 
> Have you actually observed any race there?

Yes.. if you run the mentioned two loops on an SMP box, you will find that
dcache_readdir is looping with dcache_lock held up in less than a minute. 
The problem comes when sysfs_remove_dir has done list_del_init on the cursor 
dentry which is being used in dcache_readdir. Please enable pr_debug() also 
in sysfs_remove_dir to see the this happening.

Maneesh

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Re: [PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[Maneesh Soni]

On Thu, Nov 20, 2003 at 11:26:55AM +0530, Maneesh Soni wrote:
> On Thu, Nov 20, 2003 at 05:49:57AM +0000, viro@parcelfarce.linux.theplanet.co.uk wrote:
> > On Thu, Nov 20, 2003 at 11:17:07AM +0530, Maneesh Soni wrote:
> > > Hi,
> > > 
> > > sysfs_remove_dir modifies d_subdirs list which results in inconsistency
> > > when there is concurrent dcache_readdir is going on. I think there
> > > is no need for sysfs_remove_dir to modify d_subdirs list and removal
> > > of dentries from d_child list is taken care in the final dput().
> > 
> > WTF?
> > 
> > All instances of ->readdir() are protected by ->i_sem on parent.  So
> > are all operations in sysfs_remove_dir().
> > 
> > Have you actually observed any race there?
> 
> Yes.. if you run the mentioned two loops on an SMP box, you will find that
> dcache_readdir is looping with dcache_lock held up in less than a minute. 
> The problem comes when sysfs_remove_dir has done list_del_init on the cursor 
> dentry which is being used in dcache_readdir. Please enable pr_debug() also 
> in sysfs_remove_dir to see the this happening.
> 
> Maneesh

Clarrifying more:

Actually race is not directly between dcache_readdir and sysfs_remove_dir but
it is like this

cpu 0						cpu 1
dcache_dir_open()
--> adds the cursor dentry

					sysfs_remove_dir()
					--> list_del_init cursor dentry

dcache_readdir()
--> loops forever on inititalized cursor dentry.


Though all these operations happen under parent's i_sem, but it is dropped 
between ->open() and ->readdir() as both are different calls. 

I think people will also agree that there is no need for sysfs_remove_dir() 
to modify d_subdirs list.

Maneesh

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Re: [PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[Andrew Morton]

Maneesh Soni <maneesh@in.ibm.com> wrote:
>
> Actually race is not directly between dcache_readdir and sysfs_remove_dir but
>  it is like this
> 
>  cpu 0						cpu 1
>  dcache_dir_open()
>  --> adds the cursor dentry
> 
>  					sysfs_remove_dir()
>  					--> list_del_init cursor dentry
> 
>  dcache_readdir()
>  --> loops forever on inititalized cursor dentry.
> 
> 
>  Though all these operations happen under parent's i_sem, but it is dropped 
>  between ->open() and ->readdir() as both are different calls. 
> 
>  I think people will also agree that there is no need for sysfs_remove_dir() 
>  to modify d_subdirs list.

Seems to me that the libfs code is fragile.

What happens if the dentry at filp->f_private_data gets moved to a
different directory after someone did dcache_dir_open()?  Does the loop
in dcache_readdir() go infinite again?

Re: [PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[Dipankar Sarma]

On Thu, Nov 20, 2003 at 07:17:09AM -0800, Andrew Morton wrote:
> Maneesh Soni <maneesh@in.ibm.com> wrote:
> >  cpu 0						cpu 1
> >  dcache_dir_open()
> >  --> adds the cursor dentry
> > 
> >  					sysfs_remove_dir()
> >  					--> list_del_init cursor dentry
> > 
> >  dcache_readdir()
> >  --> loops forever on inititalized cursor dentry.
> > 
> > 
> >  Though all these operations happen under parent's i_sem, but it is dropped 
> >  between ->open() and ->readdir() as both are different calls. 
> > 
> >  I think people will also agree that there is no need for sysfs_remove_dir() 
> >  to modify d_subdirs list.
> 
> Seems to me that the libfs code is fragile.
> 
> What happens if the dentry at filp->f_private_data gets moved to a
> different directory after someone did dcache_dir_open()?  Does the loop
> in dcache_readdir() go infinite again?

I am not sure if it is that bad. AFAICS, private_data points to a dummy
dentry marking the current directory read position, that dentry cannot
be moved to a different directory.

Thanks
Dipankar

Re: [PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[viro]

On Thu, Nov 20, 2003 at 07:17:09AM -0800, Andrew Morton wrote:

> Seems to me that the libfs code is fragile.
> 
> What happens if the dentry at filp->f_private_data gets moved to a
> different directory after someone did dcache_dir_open()?  Does the loop
> in dcache_readdir() go infinite again?

Can't happen - it's not hashed and not attached to any inode.  IOW, there's
no way to find that animal, let alone pass it to d_move().

Re: [PATCH] sysfs_remove_dir Vs dcache_readdir race fix

[viro]

On Thu, Nov 20, 2003 at 03:55:25PM +0530, Maneesh Soni wrote:
> Actually race is not directly between dcache_readdir and sysfs_remove_dir but
> it is like this
> 
> cpu 0						cpu 1
> dcache_dir_open()
> --> adds the cursor dentry
> 
> 					sysfs_remove_dir()
> 					--> list_del_init cursor dentry
> 
> dcache_readdir()
> --> loops forever on inititalized cursor dentry.

Yes.  Thanks for spotting...

> Though all these operations happen under parent's i_sem, but it is dropped 
> between ->open() and ->readdir() as both are different calls. 
> 
> I think people will also agree that there is no need for sysfs_remove_dir() 
> to modify d_subdirs list.

Agreed.