svn commit: r341677 - stable/12/sys/net

[Andrey V. Elsukov]

Author: ae
Date: Fri Dec  7 09:38:25 2018
New Revision: 341677
URL: https://svnweb.freebsd.org/changeset/base/341677

Log:
  MFC r341008:
    Fix possible panic during ifnet detach in rtsock.
  
    The panic can happen, when some application does dump of routing table
    using sysctl interface. To prevent this, set IFF_DYING flag in
    if_detach_internal() function, when ifnet under lock is removed from
    the chain. In sysctl_rtsock() take IFNET_RLOCK_NOSLEEP() to prevent
    ifnet detach during routes enumeration. In case, if some interface was
    detached in the time before we take the lock, add the check, that ifnet
    is not DYING. This prevents access to memory that could be freed after
    ifnet is unlinked.
  
    Differential Revision:	https://reviews.freebsd.org/D18338
  
  MFC r341334:
    Adapt the fix in r341008 to correctly work with EBR.
  
    IFNET_RLOCK_NOSLEEP() is epoch_enter_preempt() in FreeBSD 12+. Holding
    it in sysctl_rtsock() doesn't protect us from ifnet unlinking, because
    unlinking occurs with IFNET_WLOCK(), that is rw_wlock+sx_xlock, and it
    doesn check that concurrent code is running in epoch section. But while
    we are in epoch section, we should be able to do access to ifnet's
    fields, even it was unlinked. Thus do not change if_addr and if_hw_addr
    fields in ifnet_detach_internal() to NULL, since rtsock code can do
    access to these fields and this is allowed while it is running in epoch
    section.
  
    This should fix the race, when ifnet_detach_internal() unlinks ifnet
    after we checked it for IFF_DYING in sysctl_dumpentry.
  
    Move free(ifp->if_hw_addr) into ifnet_free_internal(). Also remove the
    NULL check for ifp->if_description, since free(9) can correctly handle
    NULL pointer.

Modified:
  stable/12/sys/net/if.c
  stable/12/sys/net/rtsock.c
Directory Properties:
  stable/12/   (props changed)

Modified: stable/12/sys/net/if.c
==============================================================================
--- stable/12/sys/net/if.c	Fri Dec  7 03:43:34 2018	(r341676)
+++ stable/12/sys/net/if.c	Fri Dec  7 09:38:25 2018	(r341677)
@@ -597,8 +597,6 @@ if_free_internal(struct ifnet *ifp)
 #ifdef MAC
 	mac_ifnet_destroy(ifp);
 #endif /* MAC */
-	if (ifp->if_description != NULL)
-		free(ifp->if_description, M_IFDESCR);
 	IF_AFDATA_DESTROY(ifp);
 	IF_ADDR_LOCK_DESTROY(ifp);
 	ifq_delete(&ifp->if_snd);
@@ -606,6 +604,8 @@ if_free_internal(struct ifnet *ifp)
 	for (int i = 0; i < IFCOUNTERS; i++)
 		counter_u64_free(ifp->if_counters[i]);
 
+	free(ifp->if_description, M_IFDESCR);
+	free(ifp->if_hw_addr, M_IFADDR);
 	free(ifp, M_IFNET);
 }
 
@@ -1068,6 +1068,8 @@ if_detach_internal(struct ifnet *ifp, int vmove, struc
 	CK_STAILQ_FOREACH(iter, &V_ifnet, if_link)
 		if (iter == ifp) {
 			CK_STAILQ_REMOVE(&V_ifnet, ifp, ifnet, if_link);
+			if (!vmove)
+				ifp->if_flags |= IFF_DYING;
 			found = 1;
 			break;
 		}
@@ -1182,14 +1184,8 @@ if_detach_internal(struct ifnet *ifp, int vmove, struc
 		if_dead(ifp);
 
 		/*
-		 * Remove link ifaddr pointer and maybe decrement if_index.
 		 * Clean up all addresses.
 		 */
-		free(ifp->if_hw_addr, M_IFADDR);
-		ifp->if_hw_addr = NULL;
-		ifp->if_addr = NULL;
-
-		/* We can now free link ifaddr. */
 		IF_ADDR_WLOCK(ifp);
 		if (!CK_STAILQ_EMPTY(&ifp->if_addrhead)) {
 			ifa = CK_STAILQ_FIRST(&ifp->if_addrhead);

Modified: stable/12/sys/net/rtsock.c
==============================================================================
--- stable/12/sys/net/rtsock.c	Fri Dec  7 03:43:34 2018	(r341676)
+++ stable/12/sys/net/rtsock.c	Fri Dec  7 09:38:25 2018	(r341677)
@@ -1559,6 +1559,8 @@ sysctl_dumpentry(struct radix_node *rn, void *vw)
 	struct rt_addrinfo info;
 	struct sockaddr_storage ss;
 
+	IFNET_RLOCK_NOSLEEP_ASSERT();
+
 	if (w->w_op == NET_RT_FLAGS && !(rt->rt_flags & w->w_arg))
 		return 0;
 	if ((rt->rt_flags & RTF_HOST) == 0
@@ -1571,7 +1573,7 @@ sysctl_dumpentry(struct radix_node *rn, void *vw)
 	info.rti_info[RTAX_NETMASK] = rtsock_fix_netmask(rt_key(rt),
 	    rt_mask(rt), &ss);
 	info.rti_info[RTAX_GENMASK] = 0;
-	if (rt->rt_ifp) {
+	if (rt->rt_ifp && !(rt->rt_ifp->if_flags & IFF_DYING)) {
 		info.rti_info[RTAX_IFP] = rt->rt_ifp->if_addr->ifa_addr;
 		info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
 		if (rt->rt_ifp->if_flags & IFF_POINTOPOINT)
@@ -1934,8 +1936,10 @@ sysctl_rtsock(SYSCTL_HANDLER_ARGS)
 			rnh = rt_tables_get_rnh(fib, i);
 			if (rnh != NULL) {
 				RIB_RLOCK(rnh); 
+				IFNET_RLOCK_NOSLEEP();
 			    	error = rnh->rnh_walktree(&rnh->head,
 				    sysctl_dumpentry, &w);
+				IFNET_RUNLOCK_NOSLEEP();
 				RIB_RUNLOCK(rnh);
 			} else if (af != 0)
 				error = EAFNOSUPPORT;