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);