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: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696
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?
On Thu, Nov 20, 2003 at 05:49:57AM +0000, [email protected] 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: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696
On Thu, Nov 20, 2003 at 11:26:55AM +0530, Maneesh Soni wrote:
> On Thu, Nov 20, 2003 at 05:49:57AM +0000, [email protected] 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: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696
Maneesh Soni <[email protected]> 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?
On Thu, Nov 20, 2003 at 07:17:09AM -0800, Andrew Morton wrote:
> Maneesh Soni <[email protected]> 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
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.
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().