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: <4F7430DA.7050702@parallels.com>
Date:	Thu, 29 Mar 2012 13:52:26 +0400
From:	Pavel Emelyanov <xemul@...allels.com>
To:	Glauber Costa <glommer@...allels.com>
CC:	Linux Netdev List <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH 2/3] tcp: Initial repair mode

On 03/28/2012 09:20 PM, Glauber Costa wrote:
>> 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.

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

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