Skip to content

Commit 5e36cf8

Browse files
Al Virogregkh
authored andcommitted
configfs: fix a deadlock in configfs_symlink()
commit 351e5d8 upstream. Configfs abuses symlink(2). Unlike the normal filesystems, it wants the target resolved at symlink(2) time, like link(2) would've done. The problem is that ->symlink() is called with the parent directory locked exclusive, so resolving the target inside the ->symlink() is easily deadlocked. Short of really ugly games in sys_symlink() itself, all we can do is to unlock the parent before resolving the target and relock it after. However, that invalidates the checks done by the caller of ->symlink(), so we have to * check that dentry is still where it used to be (it couldn't have been moved, but it could've been unhashed) * recheck that it's still negative (somebody else might've successfully created a symlink with the same name while we were looking the target up) * recheck the permissions on the parent directory. Cc: [email protected] Signed-off-by: Al Viro <[email protected]> Signed-off-by: Christoph Hellwig <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent 0dfc45b commit 5e36cf8

File tree

1 file changed

+32
-1
lines changed

1 file changed

+32
-1
lines changed

fs/configfs/symlink.c

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,11 +157,42 @@ int configfs_symlink(struct inode *dir, struct dentry *dentry, const char *symna
157157
!type->ct_item_ops->allow_link)
158158
goto out_put;
159159

160+
/*
161+
* This is really sick. What they wanted was a hybrid of
162+
* link(2) and symlink(2) - they wanted the target resolved
163+
* at syscall time (as link(2) would've done), be a directory
164+
* (which link(2) would've refused to do) *AND* be a deep
165+
* fucking magic, making the target busy from rmdir POV.
166+
* symlink(2) is nothing of that sort, and the locking it
167+
* gets matches the normal symlink(2) semantics. Without
168+
* attempts to resolve the target (which might very well
169+
* not even exist yet) done prior to locking the parent
170+
* directory. This perversion, OTOH, needs to resolve
171+
* the target, which would lead to obvious deadlocks if
172+
* attempted with any directories locked.
173+
*
174+
* Unfortunately, that garbage is userland ABI and we should've
175+
* said "no" back in 2005. Too late now, so we get to
176+
* play very ugly games with locking.
177+
*
178+
* Try *ANYTHING* of that sort in new code, and you will
179+
* really regret it. Just ask yourself - what could a BOFH
180+
* do to me and do I want to find it out first-hand?
181+
*
182+
* AV, a thoroughly annoyed bastard.
183+
*/
184+
inode_unlock(dir);
160185
ret = get_target(symname, &path, &target_item, dentry->d_sb);
186+
inode_lock(dir);
161187
if (ret)
162188
goto out_put;
163189

164-
ret = type->ct_item_ops->allow_link(parent_item, target_item);
190+
if (dentry->d_inode || d_unhashed(dentry))
191+
ret = -EEXIST;
192+
else
193+
ret = inode_permission(dir, MAY_WRITE | MAY_EXEC);
194+
if (!ret)
195+
ret = type->ct_item_ops->allow_link(parent_item, target_item);
165196
if (!ret) {
166197
mutex_lock(&configfs_symlink_mutex);
167198
ret = create_link(parent_item, target_item, dentry);

0 commit comments

Comments
 (0)