Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

ext4: fix races in ext4_sync_parent()

Fix problems if fsync() races against a rename of a parent directory
as pointed out by Al Viro in his own inimitable way:

>While we are at it, could somebody please explain what the hell is ext4
>doing in
>static int ext4_sync_parent(struct inode *inode)
>{
> struct writeback_control wbc;
> struct dentry *dentry = NULL;
> int ret = 0;
>
> while (inode && ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY)) {
> ext4_clear_inode_state(inode, EXT4_STATE_NEWENTRY);
> dentry = list_entry(inode->i_dentry.next,
> struct dentry, d_alias);
> if (!dentry || !dentry->d_parent || !dentry->d_parent->d_inode)
> break;
> inode = dentry->d_parent->d_inode;
> ret = sync_mapping_buffers(inode->i_mapping);
> ...
>Note that dentry obviously can't be NULL there. dentry->d_parent is never
>NULL. And dentry->d_parent would better not be negative, for crying out
>loud! What's worse, there's no guarantees that dentry->d_parent will
>remain our parent over that sync_mapping_buffers() *and* that inode won't
>just be freed under us (after rename() and memory pressure leading to
>eviction of what used to be our dentry->d_parent)......

Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

+21 -5
+21 -5
fs/ext4/fsync.c
··· 129 129 { 130 130 struct writeback_control wbc; 131 131 struct dentry *dentry = NULL; 132 + struct inode *next; 132 133 int ret = 0; 133 134 134 - while (inode && ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY)) { 135 + if (!ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY)) 136 + return 0; 137 + inode = igrab(inode); 138 + while (ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY)) { 135 139 ext4_clear_inode_state(inode, EXT4_STATE_NEWENTRY); 136 - dentry = list_entry(inode->i_dentry.next, 137 - struct dentry, d_alias); 138 - if (!dentry || !dentry->d_parent || !dentry->d_parent->d_inode) 140 + dentry = NULL; 141 + spin_lock(&inode->i_lock); 142 + if (!list_empty(&inode->i_dentry)) { 143 + dentry = list_first_entry(&inode->i_dentry, 144 + struct dentry, d_alias); 145 + dget(dentry); 146 + } 147 + spin_unlock(&inode->i_lock); 148 + if (!dentry) 139 149 break; 140 - inode = dentry->d_parent->d_inode; 150 + next = igrab(dentry->d_parent->d_inode); 151 + dput(dentry); 152 + if (!next) 153 + break; 154 + iput(inode); 155 + inode = next; 141 156 ret = sync_mapping_buffers(inode->i_mapping); 142 157 if (ret) 143 158 break; ··· 163 148 if (ret) 164 149 break; 165 150 } 151 + iput(inode); 166 152 return ret; 167 153 } 168 154