From a946bc48542e72d387e36143fc640b4c10c4abd0 Mon Sep 17 00:00:00 2001 From: sam Date: Sat, 8 Nov 2003 22:59:22 +0000 Subject: o correct locking problem: the inpcb must be held across tcp_respond o add assertions in tcp_respond to validate inpcb locking assumptions o use local variable instead of chasing pointers in tcp_respond Supported by: FreeBSD Foundation --- sys/netinet/tcp_input.c | 6 +++--- sys/netinet/tcp_reass.c | 6 +++--- sys/netinet/tcp_subr.c | 35 ++++++++++++++++++++--------------- sys/netinet/tcp_timewait.c | 35 ++++++++++++++++++++--------------- 4 files changed, 46 insertions(+), 36 deletions(-) (limited to 'sys') diff --git a/sys/netinet/tcp_input.c b/sys/netinet/tcp_input.c index 21d530f..97f7f3c 100644 --- a/sys/netinet/tcp_input.c +++ b/sys/netinet/tcp_input.c @@ -2348,9 +2348,6 @@ dropwithreset: &tcp_savetcp, 0); #endif - if (tp) - INP_UNLOCK(inp); - if (thflags & TH_ACK) /* mtod() below is safe as long as hdr dropping is delayed */ tcp_respond(tp, mtod(m, void *), th, m, (tcp_seq)0, th->th_ack, @@ -2362,6 +2359,9 @@ dropwithreset: tcp_respond(tp, mtod(m, void *), th, m, th->th_seq+tlen, (tcp_seq)0, TH_RST|TH_ACK); } + + if (tp) + INP_UNLOCK(inp); if (headlocked) INP_INFO_WUNLOCK(&tcbinfo); return; diff --git a/sys/netinet/tcp_reass.c b/sys/netinet/tcp_reass.c index 21d530f..97f7f3c 100644 --- a/sys/netinet/tcp_reass.c +++ b/sys/netinet/tcp_reass.c @@ -2348,9 +2348,6 @@ dropwithreset: &tcp_savetcp, 0); #endif - if (tp) - INP_UNLOCK(inp); - if (thflags & TH_ACK) /* mtod() below is safe as long as hdr dropping is delayed */ tcp_respond(tp, mtod(m, void *), th, m, (tcp_seq)0, th->th_ack, @@ -2362,6 +2359,9 @@ dropwithreset: tcp_respond(tp, mtod(m, void *), th, m, th->th_seq+tlen, (tcp_seq)0, TH_RST|TH_ACK); } + + if (tp) + INP_UNLOCK(inp); if (headlocked) INP_INFO_WUNLOCK(&tcbinfo); return; diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c index 78621e7..7ce06f6 100644 --- a/sys/netinet/tcp_subr.c +++ b/sys/netinet/tcp_subr.c @@ -378,6 +378,7 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) int isipv6; #endif /* INET6 */ int ipflags = 0; + struct inpcb *inp; KASSERT(tp != NULL || m != NULL, ("tcp_respond: tp and m both NULL")); @@ -388,18 +389,23 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) ip = ipgen; if (tp) { + inp = tp->t_inpcb; + KASSERT(inp != NULL, ("tcp control block w/o inpcb")); + INP_INFO_WLOCK_ASSERT(&tcbinfo); + INP_LOCK_ASSERT(inp); if (!(flags & TH_RST)) { - win = sbspace(&tp->t_inpcb->inp_socket->so_rcv); + win = sbspace(&inp->inp_socket->so_rcv); if (win > (long)TCP_MAXWIN << tp->rcv_scale) win = (long)TCP_MAXWIN << tp->rcv_scale; } #ifdef INET6 if (isipv6) - ro6 = &tp->t_inpcb->in6p_route; + ro6 = &inp->in6p_route; else #endif /* INET6 */ - ro = &tp->t_inpcb->inp_route; + ro = &inp->inp_route; } else { + inp = NULL; #ifdef INET6 if (isipv6) { ro6 = &sro6; @@ -480,12 +486,12 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) m->m_pkthdr.len = tlen; m->m_pkthdr.rcvif = (struct ifnet *) 0; #ifdef MAC - if (tp != NULL && tp->t_inpcb != NULL) { + if (inp != NULL) { /* * Packet is associated with a socket, so allow the * label of the response to reflect the socket label. */ - mac_create_mbuf_from_socket(tp->t_inpcb->inp_socket, m); + mac_create_mbuf_from_socket(inp->inp_socket, m); } else { /* * Packet is not associated with a socket, so possibly @@ -510,7 +516,7 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) nth->th_sum = in6_cksum(m, IPPROTO_TCP, sizeof(struct ip6_hdr), tlen - sizeof(struct ip6_hdr)); - ip6->ip6_hlim = in6_selecthlim(tp ? tp->t_inpcb : NULL, + ip6->ip6_hlim = in6_selecthlim(inp, ro6 && ro6->ro_rt ? ro6->ro_rt->rt_ifp : NULL); @@ -523,26 +529,25 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) m->m_pkthdr.csum_data = offsetof(struct tcphdr, th_sum); } #ifdef TCPDEBUG - if (tp == NULL || (tp->t_inpcb->inp_socket->so_options & SO_DEBUG)) + if (tp == NULL || (inp->inp_socket->so_options & SO_DEBUG)) tcp_trace(TA_OUTPUT, 0, tp, mtod(m, void *), th, 0); #endif #ifdef INET6 if (isipv6) { - (void)ip6_output(m, NULL, ro6, ipflags, NULL, NULL, - tp ? tp->t_inpcb : NULL); + (void) ip6_output(m, NULL, ro6, ipflags, NULL, NULL, inp); if (ro6 == &sro6 && ro6->ro_rt) { RTFREE(ro6->ro_rt); ro6->ro_rt = NULL; } } else #endif /* INET6 */ - { - (void) ip_output(m, NULL, ro, ipflags, NULL, tp ? tp->t_inpcb : NULL); - if (ro == &sro && ro->ro_rt) { - RTFREE(ro->ro_rt); - ro->ro_rt = NULL; + { + (void) ip_output(m, NULL, ro, ipflags, NULL, inp); + if (ro == &sro && ro->ro_rt) { + RTFREE(ro->ro_rt); + ro->ro_rt = NULL; + } } - } } /* diff --git a/sys/netinet/tcp_timewait.c b/sys/netinet/tcp_timewait.c index 78621e7..7ce06f6 100644 --- a/sys/netinet/tcp_timewait.c +++ b/sys/netinet/tcp_timewait.c @@ -378,6 +378,7 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) int isipv6; #endif /* INET6 */ int ipflags = 0; + struct inpcb *inp; KASSERT(tp != NULL || m != NULL, ("tcp_respond: tp and m both NULL")); @@ -388,18 +389,23 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) ip = ipgen; if (tp) { + inp = tp->t_inpcb; + KASSERT(inp != NULL, ("tcp control block w/o inpcb")); + INP_INFO_WLOCK_ASSERT(&tcbinfo); + INP_LOCK_ASSERT(inp); if (!(flags & TH_RST)) { - win = sbspace(&tp->t_inpcb->inp_socket->so_rcv); + win = sbspace(&inp->inp_socket->so_rcv); if (win > (long)TCP_MAXWIN << tp->rcv_scale) win = (long)TCP_MAXWIN << tp->rcv_scale; } #ifdef INET6 if (isipv6) - ro6 = &tp->t_inpcb->in6p_route; + ro6 = &inp->in6p_route; else #endif /* INET6 */ - ro = &tp->t_inpcb->inp_route; + ro = &inp->inp_route; } else { + inp = NULL; #ifdef INET6 if (isipv6) { ro6 = &sro6; @@ -480,12 +486,12 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) m->m_pkthdr.len = tlen; m->m_pkthdr.rcvif = (struct ifnet *) 0; #ifdef MAC - if (tp != NULL && tp->t_inpcb != NULL) { + if (inp != NULL) { /* * Packet is associated with a socket, so allow the * label of the response to reflect the socket label. */ - mac_create_mbuf_from_socket(tp->t_inpcb->inp_socket, m); + mac_create_mbuf_from_socket(inp->inp_socket, m); } else { /* * Packet is not associated with a socket, so possibly @@ -510,7 +516,7 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) nth->th_sum = in6_cksum(m, IPPROTO_TCP, sizeof(struct ip6_hdr), tlen - sizeof(struct ip6_hdr)); - ip6->ip6_hlim = in6_selecthlim(tp ? tp->t_inpcb : NULL, + ip6->ip6_hlim = in6_selecthlim(inp, ro6 && ro6->ro_rt ? ro6->ro_rt->rt_ifp : NULL); @@ -523,26 +529,25 @@ tcp_respond(tp, ipgen, th, m, ack, seq, flags) m->m_pkthdr.csum_data = offsetof(struct tcphdr, th_sum); } #ifdef TCPDEBUG - if (tp == NULL || (tp->t_inpcb->inp_socket->so_options & SO_DEBUG)) + if (tp == NULL || (inp->inp_socket->so_options & SO_DEBUG)) tcp_trace(TA_OUTPUT, 0, tp, mtod(m, void *), th, 0); #endif #ifdef INET6 if (isipv6) { - (void)ip6_output(m, NULL, ro6, ipflags, NULL, NULL, - tp ? tp->t_inpcb : NULL); + (void) ip6_output(m, NULL, ro6, ipflags, NULL, NULL, inp); if (ro6 == &sro6 && ro6->ro_rt) { RTFREE(ro6->ro_rt); ro6->ro_rt = NULL; } } else #endif /* INET6 */ - { - (void) ip_output(m, NULL, ro, ipflags, NULL, tp ? tp->t_inpcb : NULL); - if (ro == &sro && ro->ro_rt) { - RTFREE(ro->ro_rt); - ro->ro_rt = NULL; + { + (void) ip_output(m, NULL, ro, ipflags, NULL, inp); + if (ro == &sro && ro->ro_rt) { + RTFREE(ro->ro_rt); + ro->ro_rt = NULL; + } } - } } /* -- cgit v1.1