git: a13039e27092 - main - inpcb: reoder inpcb destruction

From: Gleb Smirnoff <glebius_at_FreeBSD.org>
Date: Wed, 27 Dec 2023 16:35:42 UTC
The branch main has been updated by glebius:

URL: https://cgit.FreeBSD.org/src/commit/?id=a13039e2709277b1c3b159e694cc909a5e044151

commit a13039e2709277b1c3b159e694cc909a5e044151
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2023-12-27 16:34:37 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2023-12-27 16:34:37 +0000

    inpcb: reoder inpcb destruction
    
    First, merge in_pcbdetach() with in_pcbfree().  The comment for
    in_pcbdetach() was no longer correct.  Then, make sure we remove
    the inpcb from the hash before we commit any destructive actions
    on it.  There are couple functions that rely on the hash lock
    skipping SMR + inpcb lock to lookup an inpcb.  Although there are
    no known functions that similarly rely on the global inpcb list
    lock, also do list removal before destructive actions.
    
    PR:                     273890
    Reviewed by:            markj
    Differential Revision:  https://reviews.freebsd.org/D43122
---
 sys/netinet/in_pcb.c       | 39 +++++++++++++++------------------------
 sys/netinet/in_pcb.h       |  1 -
 sys/netinet/raw_ip.c       |  1 -
 sys/netinet/tcp_syncache.c |  2 --
 sys/netinet/tcp_usrreq.c   |  2 --
 sys/netinet/udp_usrreq.c   |  1 -
 sys/netinet6/raw_ip6.c     |  1 -
 sys/netinet6/udp6_usrreq.c |  1 -
 8 files changed, 15 insertions(+), 33 deletions(-)

diff --git a/sys/netinet/in_pcb.c b/sys/netinet/in_pcb.c
index 0d763184f68c..63b4fc57230e 100644
--- a/sys/netinet/in_pcb.c
+++ b/sys/netinet/in_pcb.c
@@ -1403,26 +1403,6 @@ in_pcbdisconnect(struct inpcb *inp)
 }
 #endif /* INET */
 
-/*
- * in_pcbdetach() is responsibe for disassociating a socket from an inpcb.
- * For most protocols, this will be invoked immediately prior to calling
- * in_pcbfree().  However, with TCP the inpcb may significantly outlive the
- * socket, in which case in_pcbfree() is deferred.
- */
-void
-in_pcbdetach(struct inpcb *inp)
-{
-
-	KASSERT(inp->inp_socket != NULL, ("%s: inp_socket == NULL", __func__));
-
-#ifdef RATELIMIT
-	if (inp->inp_snd_tag != NULL)
-		in_pcbdetach_txrtlmt(inp);
-#endif
-	inp->inp_socket->so_pcb = NULL;
-	inp->inp_socket = NULL;
-}
-
 /*
  * inpcb hash lookups are protected by SMR section.
  *
@@ -1733,19 +1713,30 @@ in_pcbfree(struct inpcb *inp)
 #endif
 
 	INP_WLOCK_ASSERT(inp);
-	KASSERT(inp->inp_socket == NULL, ("%s: inp_socket != NULL", __func__));
+	KASSERT(inp->inp_socket != NULL, ("%s: inp_socket == NULL", __func__));
 	KASSERT((inp->inp_flags & INP_FREED) == 0,
 	    ("%s: called twice for pcb %p", __func__, inp));
 
-	inp->inp_flags |= INP_FREED;
+	/*
+	 * in_pcblookup_local() and in6_pcblookup_local() may return an inpcb
+	 * from the hash without acquiring inpcb lock, they rely on the hash
+	 * lock, thus in_pcbremhash() should be the first action.
+	 */
+	if (inp->inp_flags & INP_INHASHLIST)
+		in_pcbremhash(inp);
 	INP_INFO_WLOCK(pcbinfo);
 	inp->inp_gencnt = ++pcbinfo->ipi_gencnt;
 	pcbinfo->ipi_count--;
 	CK_LIST_REMOVE(inp, inp_list);
 	INP_INFO_WUNLOCK(pcbinfo);
 
-	if (inp->inp_flags & INP_INHASHLIST)
-		in_pcbremhash(inp);
+#ifdef RATELIMIT
+	if (inp->inp_snd_tag != NULL)
+		in_pcbdetach_txrtlmt(inp);
+#endif
+	inp->inp_flags |= INP_FREED;
+	inp->inp_socket->so_pcb = NULL;
+	inp->inp_socket = NULL;
 
 	RO_INVALIDATE_CACHE(&inp->inp_route);
 #ifdef MAC
diff --git a/sys/netinet/in_pcb.h b/sys/netinet/in_pcb.h
index adde89dde873..3d633741dc27 100644
--- a/sys/netinet/in_pcb.h
+++ b/sys/netinet/in_pcb.h
@@ -670,7 +670,6 @@ int	in_pcbconnect(struct inpcb *, struct sockaddr_in *, struct ucred *,
 	    bool);
 int	in_pcbconnect_setup(struct inpcb *, struct sockaddr_in *, in_addr_t *,
 	    u_short *, in_addr_t *, u_short *, struct ucred *);
-void	in_pcbdetach(struct inpcb *);
 void	in_pcbdisconnect(struct inpcb *);
 void	in_pcbdrop(struct inpcb *);
 void	in_pcbfree(struct inpcb *);
diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c
index 2a1e3dfb8616..4a61e685d898 100644
--- a/sys/netinet/raw_ip.c
+++ b/sys/netinet/raw_ip.c
@@ -860,7 +860,6 @@ rip_detach(struct socket *so)
 		ip_rsvp_force_done(so);
 	if (so == V_ip_rsvpd)
 		ip_rsvp_done();
-	in_pcbdetach(inp);
 	in_pcbfree(inp);
 }
 
diff --git a/sys/netinet/tcp_syncache.c b/sys/netinet/tcp_syncache.c
index 2c381ef600d6..20c77930556e 100644
--- a/sys/netinet/tcp_syncache.c
+++ b/sys/netinet/tcp_syncache.c
@@ -803,7 +803,6 @@ syncache_socket(struct syncache *sc, struct socket *lso, struct mbuf *m)
 	}
 	inp = sotoinpcb(so);
 	if ((tp = tcp_newtcpcb(inp)) == NULL) {
-		in_pcbdetach(inp);
 		in_pcbfree(inp);
 		sodealloc(so);
 		goto allocfail;
@@ -1051,7 +1050,6 @@ allocfail:
 	return (NULL);
 
 abort:
-	in_pcbdetach(inp);
 	in_pcbfree(inp);
 	sodealloc(so);
 	if ((s = tcp_log_addrs(&sc->sc_inc, NULL, NULL, NULL))) {
diff --git a/sys/netinet/tcp_usrreq.c b/sys/netinet/tcp_usrreq.c
index d3ba42fd9d06..dad79374c08b 100644
--- a/sys/netinet/tcp_usrreq.c
+++ b/sys/netinet/tcp_usrreq.c
@@ -175,7 +175,6 @@ tcp_usr_attach(struct socket *so, int proto, struct thread *td)
 	tp = tcp_newtcpcb(inp);
 	if (tp == NULL) {
 		error = ENOBUFS;
-		in_pcbdetach(inp);
 		in_pcbfree(inp);
 		goto out;
 	}
@@ -213,7 +212,6 @@ tcp_usr_detach(struct socket *so)
 	    ("%s: inp %p not dropped or embryonic", __func__, inp));
 
 	tcp_discardcb(tp);
-	in_pcbdetach(inp);
 	in_pcbfree(inp);
 }
 
diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
index 77cbf9d98c83..f7e0f7417818 100644
--- a/sys/netinet/udp_usrreq.c
+++ b/sys/netinet/udp_usrreq.c
@@ -1641,7 +1641,6 @@ udp_detach(struct socket *so)
 	KASSERT(inp->inp_faddr.s_addr == INADDR_ANY,
 	    ("udp_detach: not disconnected"));
 	INP_WLOCK(inp);
-	in_pcbdetach(inp);
 	in_pcbfree(inp);
 }
 
diff --git a/sys/netinet6/raw_ip6.c b/sys/netinet6/raw_ip6.c
index 266925836329..174cc29e6008 100644
--- a/sys/netinet6/raw_ip6.c
+++ b/sys/netinet6/raw_ip6.c
@@ -687,7 +687,6 @@ rip6_detach(struct socket *so)
 	/* xxx: RSVP */
 	INP_WLOCK(inp);
 	free(inp->in6p_icmp6filt, M_PCB);
-	in_pcbdetach(inp);
 	in_pcbfree(inp);
 }
 
diff --git a/sys/netinet6/udp6_usrreq.c b/sys/netinet6/udp6_usrreq.c
index 681645880368..bbd7ba9477d6 100644
--- a/sys/netinet6/udp6_usrreq.c
+++ b/sys/netinet6/udp6_usrreq.c
@@ -1201,7 +1201,6 @@ udp6_detach(struct socket *so)
 	KASSERT(inp != NULL, ("udp6_detach: inp == NULL"));
 
 	INP_WLOCK(inp);
-	in_pcbdetach(inp);
 	in_pcbfree(inp);
 }