possible race condition in vfs code
I think I found a race condition in some of the inode handling code. Here's where I think it is:
1. Task A is inside kill_super(). It clears the MS_ACTIVE
flag of the s_flags field of the super_block struct and
calls invalidate_inodes(). In invalidate_inodes(), it
attempts to acquire inode_lock and spins because task B,
executing inside iput(), just decremented the reference
count of some inode i to 0 and acquired inode_lock.
2. Then the "if (!inode->i_nlink)" test condition evaluates
to false for task B so it executes the following chunk
of code:
01 } else {
02 if (!list_empty(&inode->i_hash)) {
03 if (!(inode->i_state & (I_DIRTY|I_LOCK))) {
04 list_del(&inode->i_list);
05 list_add(&inode->i_list, &inode_unused);
06 }
07 inodes_stat.nr_unused++;
08 spin_unlock(&inode_lock);
09 if (!sb || (sb->s_flags & MS_ACTIVE))
10 return;
11 write_inode_now(inode, 1);
12 spin_lock(&inode_lock);
13 inodes_stat.nr_unused--;
14 list_del_init(&inode->i_hash);
15 }
16 list_del_init(&inode->i_list);
17 inode->i_state|=I_FREEING;
18 inodes_stat.nr_inodes--;
19 spin_unlock(&inode_lock);
20 if (inode->i_data.nrpages)
21 truncate_inode_pages(&inode->i_data, 0);
22 clear_inode(inode);
23 }
24 destroy_inode(inode);
Now the test condition on line 02 evaluates to true, so
task B adds the inode to the inode_unused list and then
releases inode_lock on line 08.
3. Now A acquires inode_lock and B spins on inode_lock inside
write_inode_now();
4. Task A calls invalidate_list(), transferring inode i from
the inode_unused list to its own private throw_away list.
5. Task A releases inode_lock, allowing B to acquire inode_lock
and continue executing.
6. A attempts to destroy inode i inside dispose_list() while B
simultaneously attempts to destroy i, potentially causing
all sorts of bad things to happen.So, did I find a bug or are my eyes playing tricks on me?
-Dave Peterson
[email protected]
P.S. Please CC my email address when responding to this message.
That piece of code looks wrong in other ways also..if we have unmounted an active fs (which shouldn't be done but happens!) we shouldn't be at least writing back to it anything! The !sb test isn't useful (we never clear it in live inodes) and MS_ACTIVE handling is racy as hell wrt umount...
--Mika
Quoted 71 lines · Dave Peterson
I think I found a race condition in some of the inode handling code. Here's where I think it is:
1. Task A is inside kill_super(). It clears the MS_ACTIVE flag of the s_flags field of the super_block struct and calls invalidate_inodes(). In invalidate_inodes(), it attempts to acquire inode_lock and spins because task B, executing inside iput(), just decremented the reference count of some inode i to 0 and acquired inode_lock. 2. Then the "if (!inode->i_nlink)" test condition evaluates to false for task B so it executes the following chunk of code: 01 } else { 02 if (!list_empty(&inode->i_hash)) { 03 if (!(inode->i_state & (I_DIRTY|I_LOCK))) { 04 list_del(&inode->i_list); 05 list_add(&inode->i_list, &inode_unused); 06 } 07 inodes_stat.nr_unused++; 08 spin_unlock(&inode_lock); 09 if (!sb || (sb->s_flags & MS_ACTIVE)) 10 return; 11 write_inode_now(inode, 1); 12 spin_lock(&inode_lock); 13 inodes_stat.nr_unused--; 14 list_del_init(&inode->i_hash); 15 } 16 list_del_init(&inode->i_list); 17 inode->i_state|=I_FREEING; 18 inodes_stat.nr_inodes--; 19 spin_unlock(&inode_lock); 20 if (inode->i_data.nrpages) 21 truncate_inode_pages(&inode->i_data, 0); 22 clear_inode(inode); 23 } 24 destroy_inode(inode); Now the test condition on line 02 evaluates to true, so task B adds the inode to the inode_unused list and then releases inode_lock on line 08. 3. Now A acquires inode_lock and B spins on inode_lock inside write_inode_now(); 4. Task A calls invalidate_list(), transferring inode i from the inode_unused list to its own private throw_away list. 5. Task A releases inode_lock, allowing B to acquire inode_lock and continue executing. 6. A attempts to destroy inode i inside dispose_list() while B simultaneously attempts to destroy i, potentially causing all sorts of bad things to happen.So, did I find a bug or are my eyes playing tricks on me?
-Dave Peterson
[email protected]
P.S. Please CC my email address when responding to this message.-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 5 lines · Mika Penttilä
That piece of code looks wrong in other ways also..if we have unmounted an active fs (which shouldn't be done but happens!) we shouldn't be at least writing back to it anything! The !sb test isn't useful (we never clear it in live inodes) and
MS_ACTIVEhandling is racy as hell wrt umount...
Would you mind actually _reading_ kill_super() and stuff called by it?
I mean busy inodes after umount doing bogus write_inode_now. Busy inodes don't pin the superblock (vfsmnt does but it's gone, otherwise we wouldn't be in kill_sb in the first place).
Dave Peterson's fix solves the double free issue, but does potentially introduce another busy inode after sb has gone.
--Mika
Quoted 16 lines · [email protected] wrote:
Quoted 3 lines · Mika Penttilä
That piece of code looks wrong in other ways also..if we have unmounted an active fs (which shouldn't be done but happens!) we shouldn't be at least writing back to it anything! The !sb test isn't useful (we never clear it in live inodes) and
MS_ACTIVEhandling is racy as hell wrt umount...Would you mind actually _reading_
kill_super()and stuff called by it? -
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/