Re: [PATCH 2/3] tcp: Initial repair mode

On 03/28/2012 09:20 PM, Glauber Costa wrote:

quoted

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 9e7f9ba..65ae921 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -1935,7 +1935,9 @@ void tcp_close(struct sock *sk, long timeout)
  	 * advertise a zero window, then kill -9 the FTP client, wheee...
  	 * Note: timeout is always zero in such a case.
  	 */
-	if (data_was_unread) {
+	if (tcp_sk(sk)->repair) {
+		sk->sk_prot->disconnect(sk, 0);
+	} else if (data_was_unread) {
  		/* Unread data was tossed, zap the connection. */
  		NET_INC_STATS_USER(sock_net(sk), LINUX_MIB_TCPABORTONCLOSE);
  		tcp_set_state(sk, TCP_CLOSE);

@@ -2074,6 +2076,8 @@ int tcp_disconnect(struct sock *sk, int flags)
  	/* ABORT function of RFC793 */
  	if (old_state == TCP_LISTEN) {
  		inet_csk_listen_stop(sk);
+	} else if (unlikely(tp->repair)) {
+		sk->sk_err = ECONNABORTED;
  	} else if (tcp_need_reset(old_state) ||
  		   (tp->snd_nxt != tp->write_seq&&
  		(1<<  old_state)&  (TCPF_CLOSING | TCPF_LAST_ACK))) {

The patch looks good in general.
Single nitpick is that maybe you should be consistent in your use of 
unlikely. All of them seems equally unlikely, so I'd say you should wrap 
both.

OK, will fix this.

quoted

+	case TCP_REPAIR:
+		if (!tcp_can_repair_sock(sk))
+			err = -EPERM;
+		else if (val == 1) {
+			tp->repair = 1;
+			sk->sk_reuse = 2;
+			tp->repair_queue = TCP_NO_QUEUE;
+		} else if (val == 0) {
+			tp->repair = 0;
+			sk->sk_reuse = 0;
+			tcp_send_window_probe(sk);
+		} else
+			err = -EINVAL;
+
+		break;
+
+	case TCP_REPAIR_QUEUE:

Don't we need to test tcp_can_repair_sock() in all of them?
I understand that TCP_REPAIR always comes before the other ones,
so that means the socket is already in repair mode. But what
should be the behavior in case the process drops privileges?
Should it still be able to continue with the repair?

I believe it should. Because this model gives us the ability to do
both -- let others repair socket in non-root mode and keep one at
hands, giving it to anybody else only when the repair is complete.

My first impression is that we need CAP_NET_ADMIN all along, so we 
should make sure it's there.

.