Hello Vitaliy,
On 01/04/20(Wed) 12:59, Vitaliy Makkoveev wrote:
> Updated diff. The idea is to use already existing pipex API for
> pipex_session creation and destruction. pppx_if now holds a reference
> to pipex_session.
This is great!
There are many things in this diff which makes it complicated to review,
at least to me. Note that I'm not really familiar with this code.
For example the panic() after the RBT_REMOVE() is now questionable since
the lock is released then retaken. Which brings a question for later:
what is the lock really protecting? That can be documented in the header
like it is done for other data structures.
So the changes includes:
- pool_get() with PR_WAITOK should never fail. This could be the first
step and also move the allocation at the beginning to make a pattern
appear.
- Allocation of `pxi_session' separately from `pxi', this creates quite
some noise because of the line changing indirection.
- Change in the error paths, which aren't correct and IMHO should be
left out for the moment.
- Use of pipex_add_session(), isn't that independent from the allocation
of `session'?
Could you help me review this by submitting smaller diffs? Thanks a
lot!
>
> Index: sys/net/if_pppx.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 if_pppx.c
> --- sys/net/if_pppx.c 1 Apr 2020 07:15:59 -0000 1.78
> +++ sys/net/if_pppx.c 1 Apr 2020 09:50:19 -0000
> @@ -155,7 +155,7 @@ struct pppx_if {
> int pxi_unit;
> struct ifnet pxi_if;
> struct pppx_dev *pxi_dev;
> - struct pipex_session pxi_session;
> + struct pipex_session *pxi_session;
> struct pipex_iface_context pxi_ifcontext;
> };
>
> @@ -655,15 +655,10 @@ int
> pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
> {
> struct pppx_if *pxi;
> - struct pipex_session *session;
> - struct pipex_hash_head *chain;
> struct ifnet *ifp;
> int unit, error = 0;
> struct in_ifaddr *ia;
> struct sockaddr_in ifaddr;
> -#ifdef PIPEX_PPPOE
> - struct ifnet *over_ifp = NULL;
> -#endif
>
> /*
> * XXX: As long as `session' is allocated as part of a `pxi'
> @@ -673,157 +668,16 @@ pppx_add_session(struct pppx_dev *pxd, s
> if (req->pr_timeout_sec != 0)
> return (EINVAL);
>
> - 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);
> - break;
> -#endif
> -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> - case PIPEX_PROTO_PPTP:
> - 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);
> - break;
> -#ifdef INET6
> - case AF_INET6:
> - if (req->pr_peer_address.ss_len != sizeof(struct
> sockaddr_in6))
> - return (EINVAL);
> - break;
> -#endif
> - default:
> - return (EPROTONOSUPPORT);
> - }
> - 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);
> - break;
> -#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
> - default:
> - return (EPROTONOSUPPORT);
> - }
> -
> pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
> - if (pxi == NULL)
> - return (ENOMEM);
> -
> - 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;
> - session->peer_session_id = req->pr_peer_session_id;
> - session->peer_mru = req->pr_peer_mru;
> - session->timeout_sec = req->pr_timeout_sec;
> - session->ppp_flags = req->pr_ppp_flags;
> - session->ppp_id = req->pr_ppp_id;
> -
> - session->ip_forward = 1;
> -
> - session->ip_address.sin_family = AF_INET;
> - session->ip_address.sin_len = sizeof(struct sockaddr_in);
> - session->ip_address.sin_addr = req->pr_ip_address;
> -
> - session->ip_netmask.sin_family = AF_INET;
> - session->ip_netmask.sin_len = sizeof(struct sockaddr_in);
> - session->ip_netmask.sin_addr = req->pr_ip_netmask;
> -
> - if (session->ip_netmask.sin_addr.s_addr == 0L)
> - session->ip_netmask.sin_addr.s_addr = 0xffffffffL;
> - session->ip_address.sin_addr.s_addr &=
> - session->ip_netmask.sin_addr.s_addr;
> -
> - if (req->pr_peer_address.ss_len > 0)
> - memcpy(&session->peer, &req->pr_peer_address,
> - MIN(req->pr_peer_address.ss_len, sizeof(session->peer)));
> - if (req->pr_local_address.ss_len > 0)
> - memcpy(&session->local, &req->pr_local_address,
> - MIN(req->pr_local_address.ss_len, sizeof(session->local)));
> -#ifdef PIPEX_PPPOE
> - if (req->pr_protocol == PIPEX_PROTO_PPPOE)
> - session->proto.pppoe.over_ifidx = over_ifp->if_index;
> -#endif
> -#ifdef PIPEX_PPTP
> - if (req->pr_protocol == PIPEX_PROTO_PPTP) {
> - struct pipex_pptp_session *sess_pptp = &session->proto.pptp;
> -
> - sess_pptp->snd_gap = 0;
> - sess_pptp->rcv_gap = 0;
> - sess_pptp->snd_una = req->pr_proto.pptp.snd_una;
> - sess_pptp->snd_nxt = req->pr_proto.pptp.snd_nxt;
> - sess_pptp->rcv_nxt = req->pr_proto.pptp.rcv_nxt;
> - sess_pptp->rcv_acked = req->pr_proto.pptp.rcv_acked;
> -
> - sess_pptp->winsz = req->pr_proto.pptp.winsz;
> - sess_pptp->maxwinsz = req->pr_proto.pptp.maxwinsz;
> - sess_pptp->peer_maxwinsz = req->pr_proto.pptp.peer_maxwinsz;
> - /* last ack number */
> - sess_pptp->ul_snd_una = sess_pptp->snd_una - 1;
> - }
> -#endif
> -#ifdef PIPEX_L2TP
> - if (req->pr_protocol == PIPEX_PROTO_L2TP) {
> - struct pipex_l2tp_session *sess_l2tp = &session->proto.l2tp;
> -
> - /* session keys */
> - sess_l2tp->tunnel_id = req->pr_proto.l2tp.tunnel_id;
> - sess_l2tp->peer_tunnel_id = req->pr_proto.l2tp.peer_tunnel_id;
> -
> - /* protocol options */
> - sess_l2tp->option_flags = req->pr_proto.l2tp.option_flags;
> -
> - /* initial state of dynamic context */
> - sess_l2tp->ns_gap = sess_l2tp->nr_gap = 0;
> - sess_l2tp->ns_nxt = req->pr_proto.l2tp.ns_nxt;
> - sess_l2tp->nr_nxt = req->pr_proto.l2tp.nr_nxt;
> - sess_l2tp->ns_una = req->pr_proto.l2tp.ns_una;
> - sess_l2tp->nr_acked = req->pr_proto.l2tp.nr_acked;
> - /* last ack number */
> - sess_l2tp->ul_ns_una = sess_l2tp->ns_una - 1;
> - }
> -#endif
> -#ifdef PIPEX_MPPE
> - if ((req->pr_ppp_flags & PIPEX_PPP_MPPE_ACCEPTED) != 0)
> - pipex_session_init_mppe_recv(session,
> - req->pr_mppe_recv.stateless, req->pr_mppe_recv.keylenbits,
> - req->pr_mppe_recv.master_key);
> - if ((req->pr_ppp_flags & PIPEX_PPP_MPPE_ENABLED) != 0)
> - pipex_session_init_mppe_send(session,
> - req->pr_mppe_send.stateless, req->pr_mppe_send.keylenbits,
> - req->pr_mppe_send.master_key);
> -
> - 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);
> - }
> - }
> -#endif
> -
> /* try to set the interface up */
> rw_enter_write(&pppx_ifs_lk);
> unit = pppx_if_next_unit();
> if (unit < 0) {
> - pool_put(pppx_if_pl, pxi);
> - error = ENOMEM;
> rw_exit_write(&pppx_ifs_lk);
> - goto out;
> + error = ENOMEM;
> + goto err_free_iface;
> }
>
> pxi->pxi_unit = unit;
> @@ -833,10 +687,9 @@ pppx_add_session(struct pppx_dev *pxd, s
>
> /* 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;
> rw_exit_write(&pppx_ifs_lk);
> - goto out;
> + error = EADDRINUSE;
> + goto err_free_iface;
> }
>
> if (RBT_INSERT(pppx_ifs, &pppx_ifs, pxi) != NULL)
> @@ -856,24 +709,21 @@ pppx_add_session(struct pppx_dev *pxd, s
> 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);
> - LIST_INSERT_HEAD(&pipex_session_list, session, session_list);
> -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> - switch (req->pr_protocol) {
> - case PIPEX_PROTO_PPTP:
> - case PIPEX_PROTO_L2TP:
> - chain = PIPEX_PEER_ADDR_HASHTABLE(
> - pipex_sockaddr_hash_key(&session->peer.sa));
> - LIST_INSERT_HEAD(chain, session, peer_addr_chain);
> - break;
> + /* pipex interface context */
> + pipex_iface_init(&pxi->pxi_ifcontext, ifp);
> + pipex_iface_start(&pxi->pxi_ifcontext);
> +
> + error = pipex_add_session(req, &pxi->pxi_ifcontext);
> + if (error) {
> + goto err_free_pipex;
> }
> -#endif
>
> - /* if first session is added, start timer */
> - if (LIST_NEXT(session, session_list) == NULL)
> - pipex_timer_start();
> + pxi->pxi_session = pipex_lookup_by_session_id(
> + pxi->pxi_key.pxik_protocol, pxi->pxi_key.pxik_session_id);
> + if (pxi->pxi_session == NULL) {
> + error = EINVAL;
> + goto err_free_pipex;
> + }
>
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> @@ -928,7 +778,20 @@ pppx_add_session(struct pppx_dev *pxd, s
> pxi->pxi_ready = 1;
> rw_exit_write(&pppx_ifs_lk);
>
> -out:
> + return (error);
> +
> +err_free_pipex:
> + NET_UNLOCK();
> + pipex_iface_fini(&pxi->pxi_ifcontext);
> + NET_LOCK();
> +err_free_iface:
> + rw_enter_write(&pppx_ifs_lk);
> + if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
> + panic("%s: pppx_ifs modified while lock was held", __func__);
> + LIST_REMOVE(pxi, pxi_list);
> + rw_exit_write(&pppx_ifs_lk);
> + pool_put(pppx_if_pl, pxi);
> +
> return (error);
> }
>
> @@ -941,7 +804,7 @@ pppx_del_session(struct pppx_dev *pxd, s
> if (pxi == NULL)
> return (EINVAL);
>
> - req->pcr_stat = pxi->pxi_session.stat;
> + req->pcr_stat = pxi->pxi_session->stat;
>
> pppx_if_destroy(pxd, pxi);
> return (0);
> @@ -970,29 +833,19 @@ pppx_if_destroy(struct pppx_dev *pxd, st
> struct pipex_session *session;
>
> NET_ASSERT_LOCKED();
> - session = &pxi->pxi_session;
> + session = pxi->pxi_session;
> ifp = &pxi->pxi_if;
>
> - LIST_REMOVE(session, id_chain);
> - LIST_REMOVE(session, session_list);
> -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> - switch (session->protocol) {
> - case PIPEX_PROTO_PPTP:
> - case PIPEX_PROTO_L2TP:
> - LIST_REMOVE(session, peer_addr_chain);
> - break;
> - }
> -#endif
> -
> - /* if final session is destroyed, stop timer */
> - if (LIST_EMPTY(&pipex_session_list))
> - pipex_timer_stop();
> -
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> if_detach(ifp);
> NET_LOCK();
>
> + pipex_destroy_session(session);
> + NET_UNLOCK();
> + pipex_iface_fini(&pxi->pxi_ifcontext);
> + NET_LOCK();
> +
> rw_enter_write(&pppx_ifs_lk);
> if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
> panic("%s: pppx_ifs modified while lock was held", __func__);
> @@ -1024,7 +877,7 @@ pppx_if_start(struct ifnet *ifp)
> ifp->if_obytes += m->m_pkthdr.len;
> ifp->if_opackets++;
>
> - pipex_ppp_output(m, &pxi->pxi_session, proto);
> + pipex_ppp_output(m, pxi->pxi_session, proto);
> }
> }
>
> @@ -1084,7 +937,7 @@ pppx_if_output(struct ifnet *ifp, struct
> }
> th = mtod(m, struct pppx_hdr *);
> th->pppx_proto = 0; /* not used */
> - th->pppx_id = pxi->pxi_session.ppp_id;
> + th->pppx_id = pxi->pxi_session->ppp_id;
> rw_enter_read(&pppx_devs_lk);
> error = mq_enqueue(&pxi->pxi_dev->pxd_svcq, m);
> if (error == 0) {
> @@ -1123,7 +976,7 @@ pppx_if_ioctl(struct ifnet *ifp, u_long
>
> case SIOCSIFMTU:
> if (ifr->ifr_mtu < 512 ||
> - ifr->ifr_mtu > pxi->pxi_session.peer_mru)
> + ifr->ifr_mtu > pxi->pxi_session->peer_mru)
> error = EINVAL;
> else
> ifp->if_mtu = ifr->ifr_mtu;
>
