missing barriers in some of unix_sock ->addr and ->path accesses

BugLink: https://bugs.launchpad.net/bugs/1837952
[ Upstream commit ae3b564179bfd06f32d051b9e5d72ce4b2a07c37 ]

Several u->addr and u->path users are not holding any locks in
common with unix_bind().  unix_state_lock() is useless for those
purposes.

u->addr is assign-once and *(u->addr) is fully set up by the time
we set u->addr (all under unix_table_lock).  u->path is also
set in the same critical area, also before setting u->addr, and
any unix_sock with ->path filled will have non-NULL ->addr.

So setting ->addr with smp_store_release() is all we need for those
"lockless" users - just have them fetch ->addr with smp_load_acquire()
and don't even bother looking at ->path if they see NULL ->addr.

Users of ->addr and ->path fall into several classes now:
    1) ones that do smp_load_acquire(u->addr) and access *(u->addr)
and u->path only if smp_load_acquire() has returned non-NULL.
    2) places holding unix_table_lock.  These are guaranteed that
*(u->addr) is seen fully initialized.  If unix_sock is in one of the
"bound" chains, so's ->path.
    3) unix_sock_destructor() using ->addr is safe.  All places
that set u->addr are guaranteed to have seen all stores *(u->addr)
while holding a reference to u and unix_sock_destructor() is called
when (atomic) refcount hits zero.
    4) unix_release_sock() using ->path is safe.  unix_bind()
is serialized wrt unix_release() (normally - by struct file
refcount), and for the instances that had ->path set by unix_bind()
unix_release_sock() comes from unix_release(), so they are fine.
Instances that had it set in unix_stream_connect() either end up
attached to a socket (in unix_accept()), in which case the call
chain to unix_release_sock() and serialization are the same as in
the previous case, or they never get accept'ed and unix_release_sock()
is called when the listener is shut down and its queue gets purged.
In that case the listener's queue lock provides the barriers needed -
unix_stream_connect() shoves our unix_sock into listener's queue
under that lock right after having set ->path and eventual
unix_release_sock() caller picks them from that queue under the
same lock right before calling unix_release_sock().
    5) unix_find_other() use of ->path is pointless, but safe -
it happens with successful lookup by (abstract) name, so ->path.dentry
is guaranteed to be NULL there.

earlier-variant-reviewed-by: "Paul E. McKenney" <paulmck@linux.ibm.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index a9ee634f3c4288d6c3dca1e3506fba3bfa83432a..1fc91103c70c23b2603d871dae42b6c002821f9f 100644 (file)
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -893,7 +893,7 @@ retry:
        addr->hash ^= sk->sk_type;
 
        __unix_remove_socket(sk);
-       u->addr = addr;
+       smp_store_release(&u->addr, addr);
        __unix_insert_socket(&unix_socket_table[addr->hash], sk);
        spin_unlock(&unix_table_lock);
        err = 0;
@@ -1063,7 +1063,7 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 
        err = 0;
        __unix_remove_socket(sk);
-       u->addr = addr;
+       smp_store_release(&u->addr, addr);
        __unix_insert_socket(list, sk);
 
 out_unlock:
@@ -1334,15 +1334,29 @@ restart:
        RCU_INIT_POINTER(newsk->sk_wq, &newu->peer_wq);
        otheru = unix_sk(other);
 
-       /* copy address information from listening to new sock*/
-       if (otheru->addr) {
-               refcount_inc(&otheru->addr->refcnt);
-               newu->addr = otheru->addr;
-       }
+       /* copy address information from listening to new sock
+        *
+        * The contents of *(otheru->addr) and otheru->path
+        * are seen fully set up here, since we have found
+        * otheru in hash under unix_table_lock.  Insertion
+        * into the hash chain we'd found it in had been done
+        * in an earlier critical area protected by unix_table_lock,
+        * the same one where we'd set *(otheru->addr) contents,
+        * as well as otheru->path and otheru->addr itself.
+        *
+        * Using smp_store_release() here to set newu->addr
+        * is enough to make those stores, as well as stores
+        * to newu->path visible to anyone who gets newu->addr
+        * by smp_load_acquire().  IOW, the same warranties
+        * as for unix_sock instances bound in unix_bind() or
+        * in unix_autobind().
+        */
        if (otheru->path.dentry) {
                path_get(&otheru->path);
                newu->path = otheru->path;
        }
+       refcount_inc(&otheru->addr->refcnt);
+       smp_store_release(&newu->addr, otheru->addr);
 
        /* Set credentials */
        copy_peercred(sk, other);
@@ -1456,7 +1470,7 @@ out:
 static int unix_getname(struct socket *sock, struct sockaddr *uaddr, int *uaddr_len, int peer)
 {
        struct sock *sk = sock->sk;
-       struct unix_sock *u;
+       struct unix_address *addr;
        DECLARE_SOCKADDR(struct sockaddr_un *, sunaddr, uaddr);
        int err = 0;
 
@@ -1471,19 +1485,15 @@ static int unix_getname(struct socket *sock, struct sockaddr *uaddr, int *uaddr_
                sock_hold(sk);
        }
 
-       u = unix_sk(sk);
-       unix_state_lock(sk);
-       if (!u->addr) {
+       addr = smp_load_acquire(&unix_sk(sk)->addr);
+       if (!addr) {
                sunaddr->sun_family = AF_UNIX;
                sunaddr->sun_path[0] = 0;
                *uaddr_len = sizeof(short);
        } else {
-               struct unix_address *addr = u->addr;
-
                *uaddr_len = addr->len;
                memcpy(sunaddr, addr->name, *uaddr_len);
        }
-       unix_state_unlock(sk);
        sock_put(sk);
 out:
        return err;
@@ -2076,11 +2086,11 @@ static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg,
 
 static void unix_copy_addr(struct msghdr *msg, struct sock *sk)
 {
-       struct unix_sock *u = unix_sk(sk);
+       struct unix_address *addr = smp_load_acquire(&unix_sk(sk)->addr);
 
-       if (u->addr) {
-               msg->msg_namelen = u->addr->len;
-               memcpy(msg->msg_name, u->addr->name, u->addr->len);
+       if (addr) {
+               msg->msg_namelen = addr->len;
+               memcpy(msg->msg_name, addr->name, addr->len);
        }
 }
 
@@ -2584,15 +2594,14 @@ static int unix_open_file(struct sock *sk)
        if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
                return -EPERM;
 
-       unix_state_lock(sk);
+       if (!smp_load_acquire(&unix_sk(sk)->addr))
+               return -ENOENT;
+
        path = unix_sk(sk)->path;
-       if (!path.dentry) {
-               unix_state_unlock(sk);
+       if (!path.dentry)
                return -ENOENT;
-       }
 
        path_get(&path);
-       unix_state_unlock(sk);
 
        fd = get_unused_fd_flags(O_CLOEXEC);
        if (fd < 0)
@@ -2832,7 +2841,7 @@ static int unix_seq_show(struct seq_file *seq, void *v)
                        (s->sk_state == TCP_ESTABLISHED ? SS_CONNECTING : SS_DISCONNECTING),
                        sock_i_ino(s));
 
-               if (u->addr) {
+               if (u->addr) {  // under unix_table_lock here
                        int i, len;
                        seq_putc(seq, ' ');
 

diff --git a/net/unix/diag.c b/net/unix/diag.c
index 384c84e83462e51d24e469515f4b52f8dcf55877..3183d9b8ab33232c6f42686677c056a58bc5d2fa 100644 (file)
--- a/net/unix/diag.c
+++ b/net/unix/diag.c
@@ -10,7 +10,8 @@
 
 static int sk_diag_dump_name(struct sock *sk, struct sk_buff *nlskb)
 {
-       struct unix_address *addr = unix_sk(sk)->addr;
+       /* might or might not have unix_table_lock */
+       struct unix_address *addr = smp_load_acquire(&unix_sk(sk)->addr);
 
        if (!addr)
                return 0;

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 67703dbe29eafb7138d050f9344438e808ab291b..3a8916aa73c4842ac66a0865bb48e814c70af6cd 100644 (file)
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -321,6 +321,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
                if (a->u.net->sk) {
                        struct sock *sk = a->u.net->sk;
                        struct unix_sock *u;
+                       struct unix_address *addr;
                        int len = 0;
                        char *p = NULL;
 
@@ -351,14 +352,15 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 #endif
                        case AF_UNIX:
                                u = unix_sk(sk);
+                               addr = smp_load_acquire(&u->addr);
+                               if (!addr)
+                                       break;
                                if (u->path.dentry) {
                                        audit_log_d_path(ab, " path=", &u->path);
                                        break;
                                }
-                               if (!u->addr)
-                                       break;
-                               len = u->addr->len-sizeof(short);
-                               p = &u->addr->name->sun_path[0];
+                               len = addr->len-sizeof(short);
+                               p = &addr->name->sun_path[0];
                                audit_log_format(ab, " path=");
                                if (*p)
                                        audit_log_untrustedstring(ab, p);