Skip to contentSkip to conversation

sysfs_remove_dir Vs dcache_readdir race fix

PATCH In review Maneesh Soni 4 participants 8 messages Nov 20, 2003 Driver core

lore
Share & tools
lore

Maneesh Soni

author
#0 lore raw

PATCH 1/1 sysfs_remove_dir Vs dcache_readdir race fix

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(-)
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_remove_dir-race-fix +1 -3
diff -puN fs/sysfs/dir.c~sysfs_remove_dir-race-fix fs/sysfs/dir.c
@@ -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);

_

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696
#1 lore raw
Quoted 6 lines · 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().

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?

author
#2 lore raw
Quoted 8 lines · On Thu, Nov 20, 2003 at 05:49:57AM +0000…
Quoted 6 lines · 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().

Quoted 6 lines

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

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696
author
#3 lore raw
Quoted 16 lines · Maneesh Soni
Quoted 8 lines · On Thu, Nov 20, 2003 at 05:49:57AM +0000…
Quoted 6 lines · 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().

Quoted 6 lines

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?

Quoted 7 lines

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

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696
#4 lore raw
Quoted 10 lines · Maneesh Soni

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

(quoted diff) +0 -0
 					sysfs_remove_dir()
 					--> list_del_init cursor dentry

Review

Quoted 10 lines
(quoted diff) +0 -0
 
 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?

#5 lore raw
Quoted 15 lines · Andrew Morton
Quoted 14 lines · Maneesh Soni

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

(quoted diff) +0 -0
 					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.
Quoted 8 lines
Quoted 2 lines

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

#6 lore raw
In reply to Andrew Morton
Quoted 5 lines · Andrew Morton

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().

#7 lore raw
In reply to Maneesh Soni
Quoted 7 lines · Maneesh Soni

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

Quoted 5 lines
					sysfs_remove_dir()
					--> list_del_init cursor dentry

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

Yes. Thanks for spotting...

Quoted 5 lines

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.