On 29/05/20(Fri) 13:22, Vitaliy Makkoveev wrote:
> 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.
The fact that you have to call if_detach() if a field isn't validated
makes me wonder if this refactoring is better than the existing logic.
> Also we cleaning `IFF_RUNNING' flag before pppx(4) session destruction
> in pppx_if_destroy().
This seems unrelated.
> Since we made `ifnet' and pipex(4) session initialization separately, we
> are more close to remove duplicated code.
Can't we do that directly? There are many blocks in this function:
- Allocate a `pxi'
- Attach an interface (`ifp')
- Insert an address on the interface
- Link the `pxi' with the interface
- Setup the session
Maybe user input validation should be done first.
> 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);
>
