From a57de0b4336e48db2811a2030bb68dba8dd09d88 Mon Sep 17 00:00:00 2001 From: Jiri Olsa Date: Wed, 8 Jul 2009 12:09:13 +0000 Subject: net: adding memory barrier to the poll and receive callbacks Adding memory barrier after the poll_wait function, paired with receive callbacks. Adding fuctions sock_poll_wait and sk_has_sleeper to wrap the memory barrier. Without the memory barrier, following race can happen. The race fires, when following code paths meet, and the tp->rcv_nxt and __add_wait_queue updates stay in CPU caches. CPU1 CPU2 sys_select receive packet ... ... __add_wait_queue update tp->rcv_nxt ... ... tp->rcv_nxt check sock_def_readable ... { schedule ... if (sk->sk_sleep && waitqueue_active(sk->sk_sleep)) wake_up_interruptible(sk->sk_sleep) ... } If there was no cache the code would work ok, since the wait_queue and rcv_nxt are opposit to each other. Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already passed the tp->rcv_nxt check and sleeps, or will get the new value for tp->rcv_nxt and will return with new data mask. In both cases the process (CPU1) is being added to the wait queue, so the waitqueue_active (CPU2) call cannot miss and will wake up CPU1. The bad case is when the __add_wait_queue changes done by CPU1 stay in its cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 will then endup calling schedule and sleep forever if there are no more data on the socket. Calls to poll_wait in following modules were ommited: net/bluetooth/af_bluetooth.c net/irda/af_irda.c net/irda/irnet/irnet_ppp.c net/mac80211/rc80211_pid_debugfs.c net/phonet/socket.c net/rds/af_rds.c net/rfkill/core.c net/sunrpc/cache.c net/sunrpc/rpc_pipe.c net/tipc/socket.c Signed-off-by: Jiri Olsa Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller --- net/unix/af_unix.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'net/unix/af_unix.c') diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 36d4e44d6233..fc3ebb906911 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -315,7 +315,7 @@ static void unix_write_space(struct sock *sk) { read_lock(&sk->sk_callback_lock); if (unix_writable(sk)) { - if (sk->sk_sleep && waitqueue_active(sk->sk_sleep)) + if (sk_has_sleeper(sk)) wake_up_interruptible_sync(sk->sk_sleep); sk_wake_async(sk, SOCK_WAKE_SPACE, POLL_OUT); } @@ -1985,7 +1985,7 @@ static unsigned int unix_poll(struct file *file, struct socket *sock, poll_table struct sock *sk = sock->sk; unsigned int mask; - poll_wait(file, sk->sk_sleep, wait); + sock_poll_wait(file, sk->sk_sleep, wait); mask = 0; /* exceptional events? */ @@ -2022,7 +2022,7 @@ static unsigned int unix_dgram_poll(struct file *file, struct socket *sock, struct sock *sk = sock->sk, *other; unsigned int mask, writable; - poll_wait(file, sk->sk_sleep, wait); + sock_poll_wait(file, sk->sk_sleep, wait); mask = 0; /* exceptional events? */ @@ -2053,7 +2053,7 @@ static unsigned int unix_dgram_poll(struct file *file, struct socket *sock, other = unix_peer_get(sk); if (other) { if (unix_peer(other) != sk) { - poll_wait(file, &unix_sk(other)->peer_wait, + sock_poll_wait(file, &unix_sk(other)->peer_wait, wait); if (unix_recvq_full(other)) writable = 0; -- cgit v1.2.3 From 8ba69ba6a324b13e1190fc31e41954d190fd4f1d Mon Sep 17 00:00:00 2001 From: Miklos Szeredi Date: Fri, 11 Sep 2009 11:31:45 -0700 Subject: net: unix: fix sending fds in multiple buffers Kalle Olavi Niemitalo reported that: "..., when one process calls sendmsg once to send 43804 bytes of data and one file descriptor, and another process then calls recvmsg three times to receive the 16032+16032+11740 bytes, each of those recvmsg calls returns the file descriptor in the ancillary data. I confirmed this with strace. The behaviour differs from Linux 2.6.26, where reportedly only one of those recvmsg calls (I think the first one) returned the file descriptor." This bug was introduced by a patch from me titled "net: unix: fix inflight counting bug in garbage collector", commit 6209344f5. And the reason is, quoting Kalle: "Before your patch, unix_attach_fds() would set scm->fp = NULL, so that if the loop in unix_stream_sendmsg() ran multiple iterations, it could not call unix_attach_fds() again. But now, unix_attach_fds() leaves scm->fp unchanged, and I think this causes it to be called multiple times and duplicate the same file descriptors to each struct sk_buff." Fix this by introducing a flag that is cleared at the start and set when the fds attached to the first buffer. The resulting code should work equivalently to the one on 2.6.26. Reported-by: Kalle Olavi Niemitalo Signed-off-by: Miklos Szeredi Signed-off-by: David S. Miller --- net/unix/af_unix.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'net/unix/af_unix.c') diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index fc3ebb906911..51ab497115eb 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1501,6 +1501,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, struct sk_buff *skb; int sent = 0; struct scm_cookie tmp_scm; + bool fds_sent = false; if (NULL == siocb->scm) siocb->scm = &tmp_scm; @@ -1562,12 +1563,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, size = min_t(int, size, skb_tailroom(skb)); memcpy(UNIXCREDS(skb), &siocb->scm->creds, sizeof(struct ucred)); - if (siocb->scm->fp) { + /* Only send the fds in the first buffer */ + if (siocb->scm->fp && !fds_sent) { err = unix_attach_fds(siocb->scm, skb); if (err) { kfree_skb(skb); goto out_err; } + fds_sent = true; } err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size); -- cgit v1.2.3