svn commit: r226113 - head/sys/netinet

[Lawrence Stewart]

Hi Andre and RE team,

I've had a patch sitting in re@'s inbox for this problem since 15th Sep 
and have been waiting for their go-ahead to commit. The patch I 
submitted is at:

http://people.freebsd.org/~lstewart/patches/misctcp/tcpreassstackfix_9.x.r225576.diff

The proposed commit message was:

##########
Use a backup (stack allocated) struct tseg_qent when we are unable to 
allocate one from the TCP reassembly UMA zone and the incoming segment 
is the one we've been waiting for (i.e. th_seq == rcv_nxt). This avoids 
TCP connections stalling when the zone limit is reached.

PR:        kern/155407
Reported by:    Slawa Olhovchenkov and Steven Hartland
Tested by:    Steven Hartland
Submitted by:    andre
Reviewed by:    jhb
Approved by:    re (?)
MFC after:    1 week
##########

I feel the logging changes should have been committed separately to the 
fix, but other than that, what you committed achieves the same thing as 
the patch I proposed.

I should have updated the ML thread to say it was submitted and awaiting 
approval, so you weren't to know.

Anyhoo, I guess I'll leave it up to you and re@ to sort out how you want 
to proceed, but wanted to make sure everyone was on the same page as RE 
would have gotten confused when you requested your patch be MFCed.

Cheers,
Lawrence

On 10/08/11 03:39, Andre Oppermann wrote:
> Author: andre
> Date: Fri Oct  7 16:39:03 2011
> New Revision: 226113
> URL: http://svn.freebsd.org/changeset/base/226113
>
> Log:
>    Prevent TCP sessions from stalling indefinitely in reassembly
>    when reaching the zone limit of reassembly queue entries.
>
>    When the zone limit was reached not even the missing segment
>    that would complete the sequence space could be processed
>    preventing the TCP session forever from making any further
>    progress.
>
>    Solve this deadlock by using a temporary on-stack queue entry
>    for the missing segment followed by an immediate dequeue again
>    by delivering the contiguous sequence space to the socket.
>
>    Add logging under net.inet.tcp.log_debug for reassembly queue
>    issues.
>
>    Reviewed by:	lsteward (previous version)
>    Tested by: 	Steven Hartland<killing-at-multiplay.co.uk>
>    MFC after:	3 days
>
> Modified:
>    head/sys/netinet/tcp_reass.c
>
> Modified: head/sys/netinet/tcp_reass.c
> ==============================================================================
> --- head/sys/netinet/tcp_reass.c	Fri Oct  7 16:09:44 2011	(r226112)
> +++ head/sys/netinet/tcp_reass.c	Fri Oct  7 16:39:03 2011	(r226113)
> @@ -177,7 +177,9 @@ tcp_reass(struct tcpcb *tp, struct tcphd
>   	struct tseg_qent *nq;
>   	struct tseg_qent *te = NULL;
>   	struct socket *so = tp->t_inpcb->inp_socket;
> +	char *s = NULL;
>   	int flags;
> +	struct tseg_qent tqs;
>
>   	INP_WLOCK_ASSERT(tp->t_inpcb);
>
> @@ -215,19 +217,40 @@ tcp_reass(struct tcpcb *tp, struct tcphd
>   		TCPSTAT_INC(tcps_rcvmemdrop);
>   		m_freem(m);
>   		*tlenp = 0;
> +		if ((s = tcp_log_addrs(&tp->t_inpcb->inp_inc, th, NULL, NULL))) {
> +			log(LOG_DEBUG, "%s; %s: queue limit reached, "
> +			    "segment dropped\n", s, __func__);
> +			free(s, M_TCPLOG);
> +		}
>   		return (0);
>   	}
>
>   	/*
>   	 * Allocate a new queue entry. If we can't, or hit the zone limit
>   	 * just drop the pkt.
> +	 *
> +	 * Use a temporary structure on the stack for the missing segment
> +	 * when the zone is exhausted. Otherwise we may get stuck.
>   	 */
>   	te = uma_zalloc(V_tcp_reass_zone, M_NOWAIT);
> -	if (te == NULL) {
> +	if (te == NULL&&  th->th_seq != tp->rcv_nxt) {
>   		TCPSTAT_INC(tcps_rcvmemdrop);
>   		m_freem(m);
>   		*tlenp = 0;
> +		if ((s = tcp_log_addrs(&tp->t_inpcb->inp_inc, th, NULL, NULL))) {
> +			log(LOG_DEBUG, "%s; %s: global zone limit reached, "
> +			    "segment dropped\n", s, __func__);
> +			free(s, M_TCPLOG);
> +		}
>   		return (0);
> +	} else if (th->th_seq == tp->rcv_nxt) {
> +		bzero(&tqs, sizeof(struct tseg_qent));
> +		te =&tqs;
> +		if ((s = tcp_log_addrs(&tp->t_inpcb->inp_inc, th, NULL, NULL))) {
> +			log(LOG_DEBUG, "%s; %s: global zone limit reached, "
> +			    "using stack for missing segment\n", s, __func__);
> +			free(s, M_TCPLOG);
> +		}
>   	}
>   	tp->t_segqlen++;
>
> @@ -304,6 +327,8 @@ tcp_reass(struct tcpcb *tp, struct tcphd
>   	if (p == NULL) {
>   		LIST_INSERT_HEAD(&tp->t_segq, te, tqe_q);
>   	} else {
> +		KASSERT(te !=&tqs, ("%s: temporary stack based entry not "
> +		    "first element in queue", __func__));
>   		LIST_INSERT_AFTER(p, te, tqe_q);
>   	}
>
> @@ -327,7 +352,8 @@ present:
>   			m_freem(q->tqe_m);
>   		else
>   			sbappendstream_locked(&so->so_rcv, q->tqe_m);
> -		uma_zfree(V_tcp_reass_zone, q);
> +		if (q !=&tqs)
> +			uma_zfree(V_tcp_reass_zone, q);
>   		tp->t_segqlen--;
>   		q = nq;
>   	} while (q&&  q->tqe_th->th_seq == tp->rcv_nxt);