Module Name:    src
Committed By:   ozaki-r
Date:           Mon Dec 15 06:52:25 UTC 2014

Modified Files:
        src/sys/net: if.c if.h

Log Message:
Introduce if_initialize and if_register as an alternative to if_attach

if_attach initializes an ifnet object and registers it to the system
(e.g., ifnet_list), however, if_attach doesn't complete the
initialization and the rest of it will be done by if_alloc_sadl
that is normally directly called by device drivers or called via
functions like ether_ifattach. So there is a race between
if_attach and if_alloc_sadl (A half-baked ifnet object may be
accessed, for example, via ioctl between them).

The aim of this fix is to register an initializing ifnet object
after completing its initializations. To this end, this fix
separates if_attach into an initialization part (if_initialize)
and a registration part (if_register) and call the latter after
if_alloc_sadl (ether_ifattach). So a typical usage of the two
new APIs is like this:

  if_initialize(ifp);  // was if_attach
  ether_ifattach(ifp, enaddr);
  if_register(ifp);

Nonetheless, changing every drivers to do so at once isn't
feasible. So we keep if_attach working as it used to be and
will change only some drivers that we need at this point.
Once we know the fix really works well, we'll change all
the others.

Some more information of the fix can be found here:
http://mail-index.netbsd.org/tech-kern/2014/12/10/msg018242.html

No objection on tech-kern and tech-net.


To generate a diff of this commit:
cvs rdiff -u -r1.306 -r1.307 src/sys/net/if.c
cvs rdiff -u -r1.183 -r1.184 src/sys/net/if.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/net/if.c
diff -u src/sys/net/if.c:1.306 src/sys/net/if.c:1.307
--- src/sys/net/if.c:1.306	Sun Dec 14 08:57:14 2014
+++ src/sys/net/if.c	Mon Dec 15 06:52:25 2014
@@ -1,4 +1,4 @@
-/*	$NetBSD: if.c,v 1.306 2014/12/14 08:57:14 martin Exp $	*/
+/*	$NetBSD: if.c,v 1.307 2014/12/15 06:52:25 ozaki-r Exp $	*/
 
 /*-
  * Copyright (c) 1999, 2000, 2001, 2008 The NetBSD Foundation, Inc.
@@ -90,7 +90,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.306 2014/12/14 08:57:14 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if.c,v 1.307 2014/12/15 06:52:25 ozaki-r Exp $");
 
 #include "opt_inet.h"
 
@@ -582,19 +582,21 @@ skip:
 }
 
 /*
- * Attach an interface to the list of "active" interfaces.
+ * Initialize an interface and assign an index for it.
+ *
+ * It must be called prior to a device specific attach routine
+ * (e.g., ether_ifattach and ieee80211_ifattach) or if_alloc_sadl,
+ * and be followed by if_register:
+ *
+ *     if_initialize(ifp);
+ *     ether_ifattach(ifp, enaddr);
+ *     if_register(ifp);
  */
 void
-if_attach(ifnet_t *ifp)
+if_initialize(ifnet_t *ifp)
 {
 	KASSERT(if_indexlim > 0);
 	TAILQ_INIT(&ifp->if_addrlist);
-	TAILQ_INSERT_TAIL(&ifnet_list, ifp, if_list);
-
-	if (ifioctl_attach(ifp) != 0)
-		panic("%s: ifioctl_attach() failed", __func__);
-
-	if_getindex(ifp);
 
 	/*
 	 * Link level name is allocated later by a separate call to
@@ -604,8 +606,6 @@ if_attach(ifnet_t *ifp)
 	if (ifp->if_snd.ifq_maxlen == 0)
 		ifp->if_snd.ifq_maxlen = ifqmaxlen;
 
-	sysctl_sndq_setup(&ifp->if_sysctl_log, ifp->if_xname, &ifp->if_snd);
-
 	ifp->if_broadcastaddr = 0; /* reliably crash if used uninitialized */
 
 	ifp->if_link_state = LINK_STATE_UNKNOWN;
@@ -632,6 +632,20 @@ if_attach(ifnet_t *ifp)
 	(void)pfil_run_hooks(if_pfil,
 	    (struct mbuf **)PFIL_IFNET_ATTACH, ifp, PFIL_IFNET);
 
+	if_getindex(ifp);
+}
+
+/*
+ * Register an interface to the list of "active" interfaces.
+ */
+void
+if_register(ifnet_t *ifp)
+{
+	if (ifioctl_attach(ifp) != 0)
+		panic("%s: ifioctl_attach() failed", __func__);
+
+	sysctl_sndq_setup(&ifp->if_sysctl_log, ifp->if_xname, &ifp->if_snd);
+
 	if (!STAILQ_EMPTY(&domains))
 		if_attachdomain1(ifp);
 
@@ -645,6 +659,19 @@ if_attach(ifnet_t *ifp)
 		callout_setfunc(ifp->if_slowtimo_ch, if_slowtimo, ifp);
 		if_slowtimo(ifp);
 	}
+
+	TAILQ_INSERT_TAIL(&ifnet_list, ifp, if_list);
+}
+
+/*
+ * Deprecated. Use if_initialize and if_register instead.
+ * See the above comment of if_initialize.
+ */
+void
+if_attach(ifnet_t *ifp)
+{
+	if_initialize(ifp);
+	if_register(ifp);
 }
 
 void

Index: src/sys/net/if.h
diff -u src/sys/net/if.h:1.183 src/sys/net/if.h:1.184
--- src/sys/net/if.h:1.183	Tue Dec  2 04:43:35 2014
+++ src/sys/net/if.h	Mon Dec 15 06:52:25 2014
@@ -1,4 +1,4 @@
-/*	$NetBSD: if.h,v 1.183 2014/12/02 04:43:35 ozaki-r Exp $	*/
+/*	$NetBSD: if.h,v 1.184 2014/12/15 06:52:25 ozaki-r Exp $	*/
 
 /*-
  * Copyright (c) 1999, 2000, 2001 The NetBSD Foundation, Inc.
@@ -864,7 +864,9 @@ void if_activate_sadl(struct ifnet *, st
     const struct sockaddr_dl *);
 void	if_set_sadl(struct ifnet *, const void *, u_char, bool);
 void	if_alloc_sadl(struct ifnet *);
-void	if_attach(struct ifnet *);
+void	if_initialize(struct ifnet *);
+void	if_register(struct ifnet *);
+void	if_attach(struct ifnet *); /* Deprecated. Use if_initialize and if_register */
 void	if_attachdomain(void);
 void	if_deactivate(struct ifnet *);
 void	if_purgeaddrs(struct ifnet *, int, void (*)(struct ifaddr *));

Reply via email to