On 2015/11/06, Dmitry Vyukov reported a deadlock involving the splice
system call and AF_UNIX sockets,
http://lists.openwall.net/netdev/2015/11/06/24
The situation was analyzed as
(a while ago) A: socketpair()
B: splice() from a pipe to /mnt/regular_file
does sb_start_write() on /mnt
C: try to freeze /mnt
wait for B to finish with /mnt
A: bind() try to bind our socket to /mnt/new_socket_name
lock our socket, see it not bound yet
decide that it needs to create something in /mnt
try to do sb_start_write() on /mnt, block (it's
waiting for C).
D: splice() from the same pipe to our socket
lock the pipe, see that socket is connected
try to lock the socket, block waiting for A
B: get around to actually feeding a chunk from
pipe to file, try to lock the pipe. Deadlock.
on 2015/11/10 by Al Viro,
http://lists.openwall.net/netdev/2015/11/10/4
The patch fixes this by removing the kern_path_create related code from
unix_mknod and executing it as part of unix_bind prior acquiring the
readlock of the socket in question. This means that A (as used above)
will sb_start_write on /mnt before it acquires the readlock, hence, it
won't indirectly block B which first did a sb_start_write and then
waited for a thread trying to acquire the readlock. Consequently, A
being blocked by C waiting for B won't cause a deadlock anymore
(effectively, both A and B acquire two locks in opposite order in the
situation described above).
Signed-off-by: Rainer Weikusat <[email protected]>
Tested-by: Dmitry Vyukov <[email protected]>
---
I also think this is a better (or at least more correct) solution than
the pretty obvious idea to record that the socket is in the process of
being bound and performing the mknod without the lock. Assuming the
first bind fails with -EADDRINUSE, a concurrent bind which might have
succeeded had it waited for the ultimate outcome of the first will
meanwhile have failed with -EINVAL despite the socket will end up
unbound.
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index b1314c0..9b3d268 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -953,32 +953,20 @@ fail:
return NULL;
}
-static int unix_mknod(const char *sun_path, umode_t mode, struct path *res)
+static int unix_mknod(struct dentry *dentry, struct path *path, umode_t mode,
+ struct path *res)
{
- struct dentry *dentry;
- struct path path;
- int err = 0;
- /*
- * Get the parent directory, calculate the hash for last
- * component.
- */
- dentry = kern_path_create(AT_FDCWD, sun_path, &path, 0);
- err = PTR_ERR(dentry);
- if (IS_ERR(dentry))
- return err;
+ int err;
- /*
- * All right, let's create it.
- */
- err = security_path_mknod(&path, dentry, mode, 0);
+ err = security_path_mknod(path, dentry, mode, 0);
if (!err) {
- err = vfs_mknod(d_inode(path.dentry), dentry, mode, 0);
+ err = vfs_mknod(d_inode(path->dentry), dentry, mode, 0);
if (!err) {
- res->mnt = mntget(path.mnt);
+ res->mnt = mntget(path->mnt);
res->dentry = dget(dentry);
}
}
- done_path_create(&path, dentry);
+
return err;
}
@@ -993,6 +981,8 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
unsigned int hash;
struct unix_address *addr;
struct hlist_head *list;
+ struct path path;
+ struct dentry *dentry;
err = -EINVAL;
if (sunaddr->sun_family != AF_UNIX)
@@ -1008,9 +998,21 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
goto out;
addr_len = err;
+ dentry = NULL;
+ if (sun_path[0]) {
+ /* Get the parent directory, calculate the hash for last
+ * component.
+ */
+ dentry = kern_path_create(AT_FDCWD, sun_path, &path, 0);
+
+ err = PTR_ERR(dentry);
+ if (IS_ERR(dentry))
+ goto out;
+ }
+
err = mutex_lock_interruptible(&u->readlock);
if (err)
- goto out;
+ goto out_path;
err = -EINVAL;
if (u->addr)
@@ -1026,11 +1028,11 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
addr->hash = hash ^ sk->sk_type;
atomic_set(&addr->refcnt, 1);
- if (sun_path[0]) {
- struct path path;
+ if (dentry) {
+ struct path u_path;
umode_t mode = S_IFSOCK |
(SOCK_INODE(sock)->i_mode & ~current_umask());
- err = unix_mknod(sun_path, mode, &path);
+ err = unix_mknod(dentry, &path, mode, &u_path);
if (err) {
if (err == -EEXIST)
err = -EADDRINUSE;
@@ -1038,9 +1040,9 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
goto out_up;
}
addr->hash = UNIX_HASH_SIZE;
- hash = d_backing_inode(path.dentry)->i_ino & (UNIX_HASH_SIZE-1);
+ hash = d_backing_inode(dentry)->i_ino & (UNIX_HASH_SIZE - 1);
spin_lock(&unix_table_lock);
- u->path = path;
+ u->path = u_path;
list = &unix_socket_table[hash];
} else {
spin_lock(&unix_table_lock);
@@ -1063,6 +1065,10 @@ out_unlock:
spin_unlock(&unix_table_lock);
out_up:
mutex_unlock(&u->readlock);
+out_path:
+ if (dentry)
+ done_path_create(&path, dentry);
+
out:
return err;
}
Hello Rainer,
On 27.12.2015 21:13, Rainer Weikusat wrote:
> -static int unix_mknod(const char *sun_path, umode_t mode, struct path *res)
> +static int unix_mknod(struct dentry *dentry, struct path *path, umode_t mode,
> + struct path *res)
> {
> - struct dentry *dentry;
> - struct path path;
> - int err = 0;
> - /*
> - * Get the parent directory, calculate the hash for last
> - * component.
> - */
> - dentry = kern_path_create(AT_FDCWD, sun_path, &path, 0);
> - err = PTR_ERR(dentry);
> - if (IS_ERR(dentry))
> - return err;
> + int err;
>
> - /*
> - * All right, let's create it.
> - */
> - err = security_path_mknod(&path, dentry, mode, 0);
> + err = security_path_mknod(path, dentry, mode, 0);
> if (!err) {
> - err = vfs_mknod(d_inode(path.dentry), dentry, mode, 0);
> + err = vfs_mknod(d_inode(path->dentry), dentry, mode, 0);
> if (!err) {
> - res->mnt = mntget(path.mnt);
> + res->mnt = mntget(path->mnt);
> res->dentry = dget(dentry);
> }
> }
> - done_path_create(&path, dentry);
> +
The reordered call to done_path_create will change the locking ordering
between the i_mutexes and the unix readlock. Can you comment on this? On
a first sight this looks like a much more dangerous change than the
original deadlock report. Can't this also conflict with splice code deep
down in vfs layer?
Thanks,
Hannes
Hannes Frederic Sowa <[email protected]> writes:
> On 27.12.2015 21:13, Rainer Weikusat wrote:
>> -static int unix_mknod(const char *sun_path, umode_t mode, struct path *res)
>> +static int unix_mknod(struct dentry *dentry, struct path *path, umode_t mode,
>> + struct path *res)
>> {
>> - struct dentry *dentry;
>> - struct path path;
>> - int err = 0;
>> - /*
>> - * Get the parent directory, calculate the hash for last
>> - * component.
>> - */
>> - dentry = kern_path_create(AT_FDCWD, sun_path, &path, 0);
>> - err = PTR_ERR(dentry);
>> - if (IS_ERR(dentry))
>> - return err;
>> + int err;
>>
>> - /*
>> - * All right, let's create it.
>> - */
>> - err = security_path_mknod(&path, dentry, mode, 0);
>> + err = security_path_mknod(path, dentry, mode, 0);
>> if (!err) {
>> - err = vfs_mknod(d_inode(path.dentry), dentry, mode, 0);
>> + err = vfs_mknod(d_inode(path->dentry), dentry, mode, 0);
>> if (!err) {
>> - res->mnt = mntget(path.mnt);
>> + res->mnt = mntget(path->mnt);
>> res->dentry = dget(dentry);
>> }
>> }
>> - done_path_create(&path, dentry);
>> +
>
> The reordered call to done_path_create will change the locking
> ordering between the i_mutexes and the unix readlock. Can you comment
> on this? On a first sight this looks like a much more dangerous change
> than the original deadlock report. Can't this also conflict with
> splice code deep down in vfs layer?
Practical consideration
-----------------------
kern_path_create acquires the i_mutex of the parent directory of the
to-be-created directory entry (via filename_create/ namei.c), as
required for reading a directory or creating a new entry in a directory
(as per Documentation/filesystems/directory-locking). A deadlock was
possible here if the thread doing the bind then blocked when trying to
acquire the readlock while the thread holding the readlock is blocked on
another lock held by a thread trying to perform an operation on the same
directory as the bind (possibly with some indirection). The only 'other
lock' which could come into play here is the pipe lock of a pipe
partaking in a splice_to_pipe from the same AF_UNIX socket. But the idea
that some thread would need to take a pipe lock prior to performing a
directory operation is quite odd (splice_from_pipe_to_directory?
openatparentoffifo?). I've also checked all existing users
of pipe_lock and at least, I didn't find one performing a directory
operation.
Theoretical consideration
-------------------------
NB: The text below represents my opinion on this after spending a few
days thinking about it (on and of, of course). Making an argument for
the opposite position is also possible.
The filesystem (namespace) is a shared namespace accessible to all
currently running threads/ processes. Whoever uses the filesystem may
have to wait for other filesystem users but threads not using it
shouldn't have to. Because of this and because the filesystem is a
pretty central facility, an operation needing 'some filesystem lock' and
also some other lock (or locks) should always acquire the filesystem
ones before any more specialized locks (as do_splice does when splicing
to a file). If 'filesystem locks' are always acquired first, there's
also no risk of a deadlock because code holding a filesystem lock is
blocked on a more specialized lock (eg, a pipe lock or the readlock
mutx) while some other thread holding the/ a more specialized lock wants
the already held filesystem lock.