This time pppx_add_session() has mixed initialisation order. It starts
to initialise pipex(4) session, then initialises `ifnet', then links
pipex(4) session, then continue to initialize `ifnet'.
pppx_add_session() can sleep and pppx_if_start() can start to work with
unlinked pipex(4) session.
Also pppx_if_destroy() can sleep and pppx_if_start() can access to this
`pxi' with unlinked session. pppx_if_start() has checking of
`IFF_RUNNING' flag but it's usesless.
Diff below does pppx_add_session() reordering. At first we initilize
and attach `ifnet', at second we initialize and link pipex(4) session
and at last we set `IFF_RUNNING' flag on `ifnet.
Also we cleaning `IFF_RUNNING' flag before pppx(4) session destruction
in pppx_if_destroy().
Since we made `ifnet' and pipex(4) session initialization separately, we
are more close to remove duplicated code.
Index: sys/net/if_pppx.c
===================================================================
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.87
diff -u -p -r1.87 if_pppx.c
--- sys/net/if_pppx.c 29 May 2020 06:51:52 -0000 1.87
+++ sys/net/if_pppx.c 29 May 2020 09:27:47 -0000
@@ -674,16 +674,111 @@ pppx_add_session(struct pppx_dev *pxd, s
* the timeout feature until this is fixed.
*/
if (req->pr_timeout_sec != 0)
- return (EINVAL);
+ return EINVAL;
+
+ pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
+
+ /* try to set the interface up */
+ unit = pppx_if_next_unit();
+ if (unit < 0) {
+ error = ENOMEM;
+ goto out;
+ }
+
+ pxi->pxi_unit = unit;
+ pxi->pxi_key.pxik_session_id = req->pr_session_id;
+ pxi->pxi_key.pxik_protocol = req->pr_protocol;
+ pxi->pxi_dev = pxd;
+
+ /* this is safe without splnet since we're not modifying it */
+ if (RBT_FIND(pppx_ifs, &pppx_ifs, pxi) != NULL) {
+ error = EADDRINUSE;
+ goto out;
+ }
+
+ if (RBT_INSERT(pppx_ifs, &pppx_ifs, pxi) != NULL)
+ panic("%s: pppx_ifs modified while lock was held", __func__);
+ LIST_INSERT_HEAD(&pxd->pxd_pxis, pxi, pxi_list);
+
+ ifp = &pxi->pxi_if;
+ snprintf(ifp->if_xname, sizeof(ifp->if_xname), "%s%d", "pppx", unit);
+ ifp->if_mtu = req->pr_peer_mru; /* XXX */
+ ifp->if_flags = IFF_POINTOPOINT | IFF_MULTICAST | IFF_UP;
+ ifp->if_start = pppx_if_start;
+ ifp->if_output = pppx_if_output;
+ ifp->if_ioctl = pppx_if_ioctl;
+ ifp->if_rtrequest = p2p_rtrequest;
+ ifp->if_type = IFT_PPP;
+ IFQ_SET_MAXLEN(&ifp->if_snd, 1);
+ ifp->if_softc = pxi;
+ /* ifp->if_rdomain = req->pr_rdomain; */
+
+ /* XXXSMP breaks atomicity */
+ NET_UNLOCK();
+ if_attach(ifp);
+ NET_LOCK();
+
+ if_addgroup(ifp, "pppx");
+ if_alloc_sadl(ifp);
+
+#if NBPFILTER > 0
+ bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t));
+#endif
+
+ /* XXX ipv6 support? how does the caller indicate it wants ipv6
+ * instead of ipv4?
+ */
+ memset(&ifaddr, 0, sizeof(ifaddr));
+ ifaddr.sin_family = AF_INET;
+ ifaddr.sin_len = sizeof(ifaddr);
+ ifaddr.sin_addr = req->pr_ip_srcaddr;
+
+ ia = malloc(sizeof (*ia), M_IFADDR, M_WAITOK | M_ZERO);
+
+ ia->ia_addr.sin_family = AF_INET;
+ ia->ia_addr.sin_len = sizeof(struct sockaddr_in);
+ ia->ia_addr.sin_addr = req->pr_ip_srcaddr;
+
+ ia->ia_dstaddr.sin_family = AF_INET;
+ ia->ia_dstaddr.sin_len = sizeof(struct sockaddr_in);
+ ia->ia_dstaddr.sin_addr = req->pr_ip_address;
+
+ ia->ia_sockmask.sin_family = AF_INET;
+ ia->ia_sockmask.sin_len = sizeof(struct sockaddr_in);
+ ia->ia_sockmask.sin_addr = req->pr_ip_netmask;
+
+ ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr);
+ ia->ia_ifa.ifa_dstaddr = sintosa(&ia->ia_dstaddr);
+ ia->ia_ifa.ifa_netmask = sintosa(&ia->ia_sockmask);
+ ia->ia_ifa.ifa_ifp = ifp;
+
+ ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr;
+
+ error = in_ifinit(ifp, ia, &ifaddr, 1);
+ if (error) {
+ printf("pppx: unable to set addresses for %s, error=%d\n",
+ ifp->if_xname, error);
+ goto out_detach;
+ }
+
+ if_addrhooks_run(ifp);
+
+ /* fake a pipex interface context */
+ pxi->pxi_ifcontext.ifnet_this = ifp;
+ pxi->pxi_ifcontext.pipexmode = PIPEX_ENABLED;
switch (req->pr_protocol) {
#ifdef PIPEX_PPPOE
case PIPEX_PROTO_PPPOE:
over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
- if (over_ifp == NULL)
- return (EINVAL);
- if (req->pr_peer_address.ss_family != AF_UNSPEC)
- return (EINVAL);
+ if (over_ifp == NULL) {
+ error = EINVAL;
+ goto out_detach;
+ }
+ if (req->pr_peer_address.ss_family != AF_UNSPEC) {
+ error = EINVAL;
+ goto out_detach;
+ }
break;
#endif
#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
@@ -691,45 +786,49 @@ pppx_add_session(struct pppx_dev *pxd, s
case PIPEX_PROTO_L2TP:
switch (req->pr_peer_address.ss_family) {
case AF_INET:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in))
- return (EINVAL);
+ if (req->pr_peer_address.ss_len !=
+ sizeof(struct sockaddr_in)) {
+ error = EINVAL;
+ goto out_detach;
+ }
break;
#ifdef INET6
case AF_INET6:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in6))
- return (EINVAL);
+ if (req->pr_peer_address.ss_len !=
+ sizeof(struct sockaddr_in6)) {
+ error = EINVAL;
+ goto out_detach;
+ }
break;
#endif
default:
- return (EPROTONOSUPPORT);
+ error = EPROTONOSUPPORT;
+ goto out_detach;
}
if (req->pr_peer_address.ss_family !=
req->pr_local_address.ss_family ||
req->pr_peer_address.ss_len !=
- req->pr_local_address.ss_len)
- return (EINVAL);
+ req->pr_local_address.ss_len) {
+ error = EINVAL;
+ goto out_detach;
+ }
break;
#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
default:
- return (EPROTONOSUPPORT);
+ error = EPROTONOSUPPORT;
+ goto out_detach;
}
session = pipex_lookup_by_session_id(req->pr_protocol,
req->pr_session_id);
- if (session)
- return (EEXIST);
-
- pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
+ if (session) {
+ error = EEXIST;
+ goto out_detach;
+ }
+ /* setup session */
session = &pxi->pxi_session;
- ifp = &pxi->pxi_if;
-
- /* fake a pipex interface context */
session->pipex_iface = &pxi->pxi_ifcontext;
- session->pipex_iface->ifnet_this = ifp;
- session->pipex_iface->pipexmode = PIPEX_ENABLED;
-
- /* setup session */
session->state = PIPEX_STATE_OPENED;
session->protocol = req->pr_protocol;
session->session_id = req->pr_session_id;
@@ -816,48 +915,12 @@ pppx_add_session(struct pppx_dev *pxd, s
if (pipex_session_is_mppe_required(session)) {
if (!pipex_session_is_mppe_enabled(session) ||
!pipex_session_is_mppe_accepted(session)) {
- pool_put(pppx_if_pl, pxi);
- return (EINVAL);
+ error = EINVAL;
+ goto out_detach;
}
}
#endif
- /* try to set the interface up */
- unit = pppx_if_next_unit();
- if (unit < 0) {
- pool_put(pppx_if_pl, pxi);
- error = ENOMEM;
- goto out;
- }
-
- pxi->pxi_unit = unit;
- pxi->pxi_key.pxik_session_id = req->pr_session_id;
- pxi->pxi_key.pxik_protocol = req->pr_protocol;
- pxi->pxi_dev = pxd;
-
- /* this is safe without splnet since we're not modifying it */
- if (RBT_FIND(pppx_ifs, &pppx_ifs, pxi) != NULL) {
- pool_put(pppx_if_pl, pxi);
- error = EADDRINUSE;
- goto out;
- }
-
- if (RBT_INSERT(pppx_ifs, &pppx_ifs, pxi) != NULL)
- panic("%s: pppx_ifs modified while lock was held", __func__);
- LIST_INSERT_HEAD(&pxd->pxd_pxis, pxi, pxi_list);
-
- snprintf(ifp->if_xname, sizeof(ifp->if_xname), "%s%d", "pppx", unit);
- ifp->if_mtu = req->pr_peer_mru; /* XXX */
- ifp->if_flags = IFF_POINTOPOINT | IFF_MULTICAST | IFF_UP;
- ifp->if_start = pppx_if_start;
- ifp->if_output = pppx_if_output;
- ifp->if_ioctl = pppx_if_ioctl;
- ifp->if_rtrequest = p2p_rtrequest;
- ifp->if_type = IFT_PPP;
- IFQ_SET_MAXLEN(&ifp->if_snd, 1);
- ifp->if_softc = pxi;
- /* ifp->if_rdomain = req->pr_rdomain; */
-
/* hook up pipex context */
chain = PIPEX_ID_HASHTABLE(session->session_id);
LIST_INSERT_HEAD(chain, session, id_chain);
@@ -877,58 +940,21 @@ pppx_add_session(struct pppx_dev *pxd, s
if (LIST_NEXT(session, session_list) == NULL)
pipex_timer_start();
- /* XXXSMP breaks atomicity */
- NET_UNLOCK();
- if_attach(ifp);
- NET_LOCK();
-
- if_addgroup(ifp, "pppx");
- if_alloc_sadl(ifp);
-
-#if NBPFILTER > 0
- bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t));
-#endif
SET(ifp->if_flags, IFF_RUNNING);
+ pxi->pxi_ready = 1;
- /* XXX ipv6 support? how does the caller indicate it wants ipv6
- * instead of ipv4?
- */
- memset(&ifaddr, 0, sizeof(ifaddr));
- ifaddr.sin_family = AF_INET;
- ifaddr.sin_len = sizeof(ifaddr);
- ifaddr.sin_addr = req->pr_ip_srcaddr;
-
- ia = malloc(sizeof (*ia), M_IFADDR, M_WAITOK | M_ZERO);
-
- ia->ia_addr.sin_family = AF_INET;
- ia->ia_addr.sin_len = sizeof(struct sockaddr_in);
- ia->ia_addr.sin_addr = req->pr_ip_srcaddr;
-
- ia->ia_dstaddr.sin_family = AF_INET;
- ia->ia_dstaddr.sin_len = sizeof(struct sockaddr_in);
- ia->ia_dstaddr.sin_addr = req->pr_ip_address;
-
- ia->ia_sockmask.sin_family = AF_INET;
- ia->ia_sockmask.sin_len = sizeof(struct sockaddr_in);
- ia->ia_sockmask.sin_addr = req->pr_ip_netmask;
-
- ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr);
- ia->ia_ifa.ifa_dstaddr = sintosa(&ia->ia_dstaddr);
- ia->ia_ifa.ifa_netmask = sintosa(&ia->ia_sockmask);
- ia->ia_ifa.ifa_ifp = ifp;
-
- ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr;
+ return 0;
- error = in_ifinit(ifp, ia, &ifaddr, 1);
- if (error) {
- printf("pppx: unable to set addresses for %s, error=%d\n",
- ifp->if_xname, error);
- } else {
- if_addrhooks_run(ifp);
- }
- pxi->pxi_ready = 1;
+out_detach:
+ /* XXXSMP breaks atomicity */
+ NET_UNLOCK();
+ if_detach(ifp);
+ NET_LOCK();
+ KASSERT(RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) != NULL);
+ LIST_REMOVE(pxi, pxi_list);
out:
+ pool_put(pppx_if_pl, pxi);
return (error);
}
@@ -1004,9 +1030,11 @@ pppx_if_destroy(struct pppx_dev *pxd, st
struct pipex_session *session;
NET_ASSERT_LOCKED();
- pxi->pxi_ready = 0;
- session = &pxi->pxi_session;
ifp = &pxi->pxi_if;
+ session = &pxi->pxi_session;
+
+ pxi->pxi_ready = 0;
+ CLR(ifp->if_flags, IFF_RUNNING);
LIST_REMOVE(session, id_chain);
LIST_REMOVE(session, session_list);
