ifnet: if_detach(): Fix races with vmove operations

The rationality is that the driver private data holds a strong reference
to the interface, and the detach operation shall never fail. Given the
vmove operation, if_vmove_loan(), if_vmove_reclaim() or vnet_if_return()
is not atomic and spans multiple steps, acquire ifnet_detach_sxlock only
for if_detach_internal() and if_vmove() is not sufficient. It is possible
that the thread running if_detach() sees stale vnet, or the vmoving is
in progress, then if_unlink_ifnet() will fail.

Fix that by extending coverage of ifnet_detach_sxlock a bit to also
cover if_unlink_ifnet(), so that the entire detach and vmove operation
is serialized.

Given it is an error when the if_unlink_ifnet() fails, and if_detach()
is a public KPI, prefer panic() over assertion on failure, to indicate
explicitly that bad thing happens. That shall also prevent potential
corrupted status of the interface, which is a bit hard to diagnose.

PR:		292993
Reviewed by:	glebius
MFC after:	5 days
Differential Revision:	https://reviews.freebsd.org/D56374

1 files changed, 17 insertions, 6 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index 8a148ba0fd06..e421dd3bd494 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -447,6 +447,7 @@ if_unlink_ifnet(struct ifnet *ifp, bool vmove)
 	struct ifnet *iter;
 	int found = 0;
 
+	sx_assert(&ifnet_detach_sxlock, SX_XLOCKED);
 	IFNET_WLOCK();
 	CK_STAILQ_FOREACH(iter, &V_ifnet, if_link)
 		if (iter == ifp) {
@@ -1028,14 +1029,23 @@ if_detach(struct ifnet *ifp)
 {
 	bool found;
 
+	/*
+	 * The driver private data holds a strong reference to the ifnet, and
+	 * it is actually the "owner", hence this routine shall never fail.
+	 *
+	 * Ideally we can loop retrying when we lose race with other threads
+	 * those run if_unlink_ifnet(). For simplicity, use ifnet_detach_sxlock
+	 * to serialize all the detach / vmove operations.
+	 */
+	sx_xlock(&ifnet_detach_sxlock);
 	CURVNET_SET_QUIET(ifp->if_vnet);
 	found = if_unlink_ifnet(ifp, false);
-	if (found) {
-		sx_xlock(&ifnet_detach_sxlock);
-		if_detach_internal(ifp, false);
-		sx_xunlock(&ifnet_detach_sxlock);
-	}
+	if (! found)
+		panic("%s: interface is not on the active list",
+		    ifp->if_xname);
+	if_detach_internal(ifp, false);
 	CURVNET_RESTORE();
+	sx_xunlock(&ifnet_detach_sxlock);
 }
 
 /*
@@ -1290,13 +1300,14 @@ if_vmove_reclaim(struct thread *td, char *ifname, int jid)
 	}
 
 	/* Get interface back from child jail/vnet. */
+	sx_xlock(&ifnet_detach_sxlock);
 	found = if_unlink_ifnet(ifp, true);
 	if (! found) {
+		sx_xunlock(&ifnet_detach_sxlock);
 		CURVNET_RESTORE();
 		prison_free(pr);
 		return (ENODEV);
 	}
-	sx_xlock(&ifnet_detach_sxlock);
 	if_vmove(ifp, vnet_dst);
 	sx_xunlock(&ifnet_detach_sxlock);
 	CURVNET_RESTORE();