netdev - Re: [fixed] [patch] Re: [bug] stuck localhost TCP connections, v2.6.26-rc3+

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]

Date:	Wed, 4 Jun 2008 09:25:38 +0300 (EEST)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	Joe Perches <joe@...ches.com>
cc:	David Miller <davem@...emloft.net>, mingo@...e.hu,
	mcmanus@...ksong.com, peterz@...radead.org,
	LKML <linux-kernel@...r.kernel.org>,
	Netdev <netdev@...r.kernel.org>, rjw@...k.pl,
	Andrew Morton <akpm@...ux-foundation.org>, johnpol@....mipt.ru
Subject: Re: [fixed] [patch] Re: [bug] stuck localhost TCP connections,
 v2.6.26-rc3+

On Tue, 3 Jun 2008, Joe Perches wrote:

> On Wed, 2008-06-04 at 02:22 +0300, Ilpo Järvinen wrote:
> > But here's somewhat more likely explanation... Only compile tested...
> > It probably needs some commenting from people who understand locking 
> > variants & details (I don't).
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index c9454f0..d21d2b9 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -4562,6 +4562,7 @@ static int tcp_defer_accept_check(struct sock *sk)
> >  	struct tcp_sock *tp = tcp_sk(sk);
> >  
> >  	if (tp->defer_tcp_accept.request) {
> > +		struct sock *listen_sk = tp->defer_tcp_accept.listen_sk;
> 
> Not commenting on the locking, but I think the code
> would be clearer if tp->defer_tcp_accept was used in
> a temporary instead.

Only fixes to net-2.6 now, as agreed. ...So I try to leave syntax cleanup 
to net-next.

Besides, there's a trap which makes your patch a lot worse than
mine... ;-) I too once tried a version without temporary until
noticed that trap.

> Perhaps:
> 
>  net/ipv4/tcp_input.c |   33 ++++++++++++++++-----------------
>  1 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index b54d9d3..f846e11 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4535,18 +4535,18 @@ static void tcp_urg(struct sock *sk, struct sk_buff *skb, struct tcphdr *th)
>  static int tcp_defer_accept_check(struct sock *sk)
>  {
>  	struct tcp_sock *tp = tcp_sk(sk);
> -
> -	if (tp->defer_tcp_accept.request) {
> -		int queued_data =  tp->rcv_nxt - tp->copied_seq;
> -		int hasfin =  !skb_queue_empty(&sk->sk_receive_queue) ?
> +	struct tcp_deferred_accept_info *tdai = &tp->defer_tcp_accept;
> +	if (tdai->request) {
> +		int queued_data = tp->rcv_nxt - tp->copied_seq;
> +		int hasfin = !skb_queue_empty(&sk->sk_receive_queue) ?
>  			tcp_hdr((struct sk_buff *)
>  				sk->sk_receive_queue.prev)->fin : 0;
>  
>  		if (queued_data && hasfin)
>  			queued_data--;
>  
> -		if (queued_data &&
> -		    tp->defer_tcp_accept.listen_sk->sk_state == TCP_LISTEN) {
> +		bh_lock_sock(tdai->listen_sk);
> +		if (queued_data && tdai->listen_sk->sk_state == TCP_LISTEN) {
>  			if (sock_flag(sk, SOCK_KEEPOPEN)) {
>  				inet_csk_reset_keepalive_timer(sk,
>  							       keepalive_time_when(tp));
> @@ -4554,23 +4554,22 @@ static int tcp_defer_accept_check(struct sock *sk)
>  				inet_csk_delete_keepalive_timer(sk);
>  			}
>  
> -			inet_csk_reqsk_queue_add(
> -				tp->defer_tcp_accept.listen_sk,
> -				tp->defer_tcp_accept.request,
> -				sk);
> +			inet_csk_reqsk_queue_add(tdai->listen_sk,
> +						 tdai->request,
> +						 sk);
>  
> -			tp->defer_tcp_accept.listen_sk->sk_data_ready(
> -				tp->defer_tcp_accept.listen_sk, 0);
> +			tdai->listen_sk->sk_data_ready(tdai->listen_sk, 0);
>  
> -			sock_put(tp->defer_tcp_accept.listen_sk);
> +			sock_put(tdai->listen_sk);
>  			sock_put(sk);
> -			tp->defer_tcp_accept.listen_sk = NULL;
> -			tp->defer_tcp_accept.request = NULL;
> -		} else if (hasfin ||
> -			   tp->defer_tcp_accept.listen_sk->sk_state != TCP_LISTEN) {
> +			tdai->listen_sk = NULL;
> +			tdai->request = NULL;

uh-oh...

> +		} else if (hasfin || tdai->listen_sk->sk_state != TCP_LISTEN) {
> +			bh_unlock_sock(tdai->listen_sk);
>  			tcp_reset(sk);
>  			return -1;
>  		}
> +		bh_unlock_sock(tdai->listen_sk);

...which will crash in here. 

NAK.

>  	}
>  	return 0;
>  }
> 
> 
> 

-- 
 i.