Index | Thread | Search

From:
Vitaliy Makkoveev <mvs@openbsd.org>
Subject:
sysctl_ifnames() move copyout(9) out of netlock
To:
Alexander Bluhm <bluhm@openbsd.org>, tech@openbsd.org
Date:
Sat, 25 Jan 2025 21:45:36 +0300

Download raw body.

Thread
  • Vitaliy Makkoveev:

    sysctl_ifnames() move copyout(9) out of netlock

Follow the ifioctl_get() way and link interfaces to temporary list
protected by `if_tmplist_lock' rwlock(9), so copyout(9) happens under
dedicated lock used only in sysctl_rtable() and ifioctl_get() paths. 

Note, this time `if_list' modification protected by netlock together
with kernel lock. This diff still uses shared netlock around first
interface list walkthrough, but it could be omitted, because this path
fully covered with kernel lock. I don't know, which lock is worse.

We have no dedicated test to trigger NET_RT_IFNAMES, so I used the
following to test it.

#include <sys/types.h>
#include <sys/socket.h>
#include <sys/sysctl.h>
#include <net/if.h>
#include <stdio.h>
#include <stdlib.h>
#include <err.h>
#include <string.h>

int
main(int argc, char *argv[])
{
	struct if_nameindex_msg *ifn;
	int mib[6];
	size_t len = 0, i;

	mib[0] = CTL_NET;
	mib[1] = PF_ROUTE;
	mib[2] = 0;
	mib[3] = 0;
	mib[4] = NET_RT_IFNAMES;
	mib[5] = 0;

	if (sysctl(mib, 6, NULL, &len, NULL, 0) < 0)
		err(1, "sysctl");
	if ((ifn = malloc(len)) == NULL)
		err(1, "malloc");
	if (sysctl(mib, 6, ifn, &len, NULL, 0) < 0)
		err(1, "sysctl");

	for(i = 0; i < len; i += sizeof(*ifn), ++ifn)
		printf("idx: %u\t\t name: %s\n", ifn->if_index, ifn->if_name);

	return 0;
}


Index: sys/net/if.h
===================================================================
RCS file: /cvs/src/sys/net/if.h,v
retrieving revision 1.217
diff -u -p -r1.217 if.h
--- sys/net/if.h	9 Jun 2024 16:25:28 -0000	1.217
+++ sys/net/if.h	25 Jan 2025 17:07:13 -0000
@@ -554,6 +554,7 @@ int	if_delgroup(struct ifnet *, const ch
 void	if_group_routechange(const struct sockaddr *, const struct sockaddr *);
 struct	ifnet *if_unit(const char *);
 struct	ifnet *if_get(unsigned int);
+struct	ifnet *if_ref(struct ifnet *);
 void	if_put(struct ifnet *);
 void	ifnewlladdr(struct ifnet *);
 void	if_congestion(void);
Index: sys/net/if_var.h
===================================================================
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.135
diff -u -p -r1.135 if_var.h
--- sys/net/if_var.h	24 Jan 2025 09:19:07 -0000	1.135
+++ sys/net/if_var.h	25 Jan 2025 17:07:13 -0000
@@ -233,6 +233,8 @@ struct ifnet {				/* and the entries */
 #define if_oqdrops	if_data_counters[ifc_oqdrops]
 #define if_noproto	if_data_counters[ifc_noproto]
 
+extern struct rwlock if_tmplist_lock;
+
 /*
  * The ifaddr structure contains information about one address
  * of an interface.  They are maintained by the different address families,
Index: sys/net/rtsock.c
===================================================================
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.378
diff -u -p -r1.378 rtsock.c
--- sys/net/rtsock.c	24 Jan 2025 09:16:55 -0000	1.378
+++ sys/net/rtsock.c	25 Jan 2025 17:07:13 -0000
@@ -2108,29 +2108,44 @@ sysctl_iflist(int af, struct walkarg *w)
 int
 sysctl_ifnames(struct walkarg *w)
 {
+	TAILQ_HEAD(, ifnet)	if_tmplist =
+	    TAILQ_HEAD_INITIALIZER(if_tmplist);
 	struct if_nameindex_msg ifn;
 	struct ifnet *ifp;
 	int error = 0;
 
+	rw_enter_write(&if_tmplist_lock);
+	NET_LOCK_SHARED();
 	/* XXX ignore tableid for now */
 	TAILQ_FOREACH(ifp, &ifnetlist, if_list) {
 		if (w->w_arg && w->w_arg != ifp->if_index)
 			continue;
+		if_ref(ifp);
+		TAILQ_INSERT_TAIL(&if_tmplist, ifp, if_tmplist);
+	}
+	NET_UNLOCK_SHARED();
+
+	TAILQ_FOREACH (ifp, &if_tmplist, if_tmplist) {
 		w->w_needed += sizeof(ifn);
 		if (w->w_where && w->w_needed <= w->w_given) {
-
 			memset(&ifn, 0, sizeof(ifn));
 			ifn.if_index = ifp->if_index;
 			strlcpy(ifn.if_name, ifp->if_xname,
 			    sizeof(ifn.if_name));
 			error = copyout(&ifn, w->w_where, sizeof(ifn));
 			if (error)
-				return (error);
+				break;
 			w->w_where += sizeof(ifn);
 		}
 	}
 
-	return (0);
+	while ((ifp = TAILQ_FIRST(&if_tmplist))) {
+		TAILQ_REMOVE(&if_tmplist, ifp, if_tmplist);
+		if_put(ifp);
+	}
+	rw_exit_write(&if_tmplist_lock);
+
+	return (error);
 }
 
 int
@@ -2242,9 +2257,7 @@ sysctl_rtable(int *name, u_int namelen, 
 		    &tableinfo, sizeof(tableinfo));
 		return (error);
 	case NET_RT_IFNAMES:
-		NET_LOCK_SHARED();
 		error = sysctl_ifnames(&w);
-		NET_UNLOCK_SHARED();
 		break;
 	case NET_RT_SOURCE:
 		tableid = w.w_arg;