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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F734864.6000007@parallels.com>
Date:	Wed, 28 Mar 2012 19:20:36 +0200
From:	Glauber Costa <glommer@...allels.com>
To:	Pavel Emelyanov <xemul@...allels.com>
CC:	Linux Netdev List <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH 2/3] tcp: Initial repair mode

> 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.

>
> +	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?

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html