Re: L2CAP non-blocking socket nasty race conditions

[Jean Tourrilhes <jt@bougret.hpl.hp.com>]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

On Thu, Feb 05, 2004 at 07:17:37PM +0100, Marcel Holtmann wrote:
> Hi Jean,
> 
> > > I had the attached patch in mind. Please check if this also works for
> > > you, because it should produce less code execution. I think we can also
> > > remove the list_empty() check from bt_sock_listen_poll(), because the
> > > list_for_each_safe() gives us the same result.
> > 
> > 	This should produce the same result. I'll try.
> 
> attached is a slight modified version of my patch.

	There is one thing I don't like about your patch : you disable
error checking for listening socket. I don't think it's right,
especially for checking socket shutdown.
	Let's take the following scenario. Multi-threaded program, one
listening l2cap socket. One thread poll/select on the socket. At some
point, the second thread close the socket. I would expect the first
thread to receive a POLLHUP. Note that you can do the same with a
single threaded program, closing the listening socket in the signal
handler.
	The attached patch is a minor modification of your patch that
should fix this issue.

> > > I already checked the IPV4 code and they also work with an accept queue,
> > > but at the moment I don't understand when they put the new socket on it
> > > and how they keep track of it.
> > 
> > 	Actually, I will double check, because I belive that it may be
> > normal behavior. More later.
> 
> The TCP code uses its own open_request structure for the accept queue,
> but I think the problem of non-blocking listen() will be the same as in
> Bluetooth.

	This is "normal" behaviour. As my prog has many active sockets
and many timers, poll just get called a lot in normal operation even
is the BT socket is inactive. False alarm.

> However the fix for the sendmsg() race is correct and must be applied.

	Glad you like it ;-)

> Regards
> 
> Marcel

	Jean



[-- Attachment #2: bt262_accept_race-3.diff --]
[-- Type: text/plain, Size: 1616 bytes --]

diff -u -p linux/net/bluetooth/af_bluetooth.j2.c linux/net/bluetooth/af_bluetooth.c
--- linux/net/bluetooth/af_bluetooth.j2.c	Wed Feb  4 18:51:55 2004
+++ linux/net/bluetooth/af_bluetooth.c	Thu Feb  5 14:43:21 2004
@@ -236,15 +236,28 @@ int bt_sock_recvmsg(struct kiocb *iocb, 
 	return err ? : copied;
 }
 
+static inline unsigned int bt_accept_poll(struct sock *parent)
+{
+	struct list_head *p, *n;
+	struct sock *sk;
+
+	list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
+		sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);
+		if (sk->sk_state == BT_CONNECTED)
+			return POLLIN | POLLRDNORM;
+	}
+
+	return 0;
+}
+
 unsigned int bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait)
 {
 	struct sock *sk = sock->sk;
-	unsigned int mask;
+	unsigned int mask = 0;
 
 	BT_DBG("sock %p, sk %p", sock, sk);
 
 	poll_wait(file, sk->sk_sleep, wait);
-	mask = 0;
 
 	if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
 		mask |= POLLERR;
@@ -253,16 +266,20 @@ unsigned int bt_sock_poll(struct file * 
 		mask |= POLLHUP;
 
 	if (!skb_queue_empty(&sk->sk_receive_queue) || 
-			!list_empty(&bt_sk(sk)->accept_q) ||
 			(sk->sk_shutdown & RCV_SHUTDOWN))
 		mask |= POLLIN | POLLRDNORM;
 
+	if (sk->sk_state == BT_LISTEN)
+		return mask | bt_accept_poll(sk);
+
 	if (sk->sk_state == BT_CLOSED)
 		mask |= POLLHUP;
 
-	if (sk->sk_state == BT_CONNECT || sk->sk_state == BT_CONNECT2)
+	if (sk->sk_state == BT_CONNECT ||
+			sk->sk_state == BT_CONNECT2 ||
+			sk->sk_state == BT_CONFIG)
 		return mask;
-	
+
 	if (sock_writeable(sk))
 		mask |= POLLOUT | POLLWRNORM | POLLWRBAND;
 	else