On 08/06/20(Mon) 15:09, Vitaliy Makkoveev wrote:
> On Mon, Jun 08, 2020 at 12:49:05PM +0300, Vitaliy Makkoveev wrote:
> > [...]
> > There is another way to rewrite pppx_add_session() and
> > pipex_add_session(). We can split pipex_add_session() to two
> > functions: pipex_init_session() and pipex_setup_session(). The first
> > will do checks and insert new session with PIPEX_STATE_INITIAL state.
> > The second will do the rest. So we can do checks before `ifnet’
> > dances in ppppx_add_session(). This time PIPEX_STATE_INITIAL declared
> > but not used, at least pipex_lookup_by_session_id() should be
> > refactored before.
> >
> > Is this direction more acceptable?
>
> Diff below illustrates this.
>
> 1. New function pipex_init_session() validates request and if it's
> accepted creates new pipex session but keeps it unlinked. This session
> is only inserted to `session_list'. Unlinked session can't be accessed
> by stack.
> 2. New function pipex_link_session() connects session to stack.
> 3. New function pipex_unlink_session() disconnects session from the
> stack.
>
> Duplicated pipex(4) code from pppx_add_session was replaced by
> pipex_init_session() and pipex_link_session(). pppx(4) related code is
> not reordered.
>
> Duplicated pipex(4) code from pppx_if_destroy() was replaced by
> pipex_unlink_session().
Awesome! Comments below.
> 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 8 Jun 2020 12:07:09 -0000
> @@ -659,14 +659,10 @@ pppx_add_session(struct pppx_dev *pxd, s
> {
> 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'
> @@ -676,151 +672,20 @@ 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);
> - }
> -
> - 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);
>
> 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);
> - }
> + pxi->pxi_ifcontext.ifnet_this = ifp;
> + pxi->pxi_ifcontext.pipexmode = PIPEX_ENABLED;
> +
> + error = pipex_init_session(session, req, &pxi->pxi_ifcontext);
Can you make pipex_init_session() not require an `iface' argument? This
way you don't need to do a pool_get/put(9) in case of error.
> + if (error) {
> + pool_put(pppx_if_pl, pxi);
> + goto out;
> }
> -#endif
>
> /* try to set the interface up */
> unit = pppx_if_next_unit();
> @@ -837,6 +702,7 @@ 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) {
> + pipex_unlink_session(session);
> pool_put(pppx_if_pl, pxi);
> error = EADDRINUSE;
> goto out;
> @@ -858,24 +724,7 @@ 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;
> - }
> -#endif
> -
> - /* if first session is added, start timer */
> - if (LIST_NEXT(session, session_list) == NULL)
> - pipex_timer_start();
> + pipex_link_session(session);
>
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> @@ -1008,20 +857,7 @@ pppx_if_destroy(struct pppx_dev *pxd, st
> 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();
> + pipex_unlink_session(session);
>
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> Index: sys/net/pipex.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.114
> diff -u -p -r1.114 pipex.c
> --- sys/net/pipex.c 31 May 2020 03:14:59 -0000 1.114
> +++ sys/net/pipex.c 8 Jun 2020 12:07:09 -0000
> @@ -258,13 +258,11 @@ pipex_ioctl(struct pipex_iface_context *
> /************************************************************************
> * Session management functions
> ************************************************************************/
> -Static int
> -pipex_add_session(struct pipex_session_req *req,
> - struct pipex_iface_context *iface)
> +int
> +pipex_init_session(struct pipex_session *session,
> + struct pipex_session_req *req, struct pipex_iface_context *iface)
> {
> - struct pipex_session *session;
> - struct pipex_hash_head *chain;
> - struct radix_node *rn;
> + struct pipex_session *session_tmp;
> #ifdef PIPEX_PPPOE
> struct ifnet *over_ifp = NULL;
> #endif
> @@ -312,14 +310,14 @@ pipex_add_session(struct pipex_session_r
> return (EPROTONOSUPPORT);
> }
>
> - session = pipex_lookup_by_session_id(req->pr_protocol,
> - req->pr_session_id);
> - if (session)
> - return (EEXIST);
> + LIST_FOREACH(session_tmp, &pipex_session_list, session_list) {
> + if (session_tmp->protocol == req->pr_protocol &&
> + session_tmp->session_id == req->pr_session_id)
> + return (EEXIST);
> + }
Why can't we keep pipex_lookup_by_session_id() here?
>
> /* prepare a new session */
> - session = pool_get(&pipex_session_pool, PR_WAITOK | PR_ZERO);
> - session->state = PIPEX_STATE_OPENED;
> + session->state = PIPEX_STATE_INITIAL;
> session->protocol = req->pr_protocol;
> session->session_id = req->pr_session_id;
> session->peer_session_id = req->pr_peer_session_id;
> @@ -396,19 +394,15 @@ pipex_add_session(struct pipex_session_r
> #endif
> #ifdef PIPEX_MPPE
> if ((req->pr_ppp_flags & PIPEX_PPP_MPPE_ACCEPTED) != 0) {
> - if (req->pr_mppe_recv.keylenbits <= 0) {
> - pool_put(&pipex_session_pool, session);
> + if (req->pr_mppe_recv.keylenbits <= 0)
> return (EINVAL);
> - }
> 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) {
> - if (req->pr_mppe_send.keylenbits <= 0) {
> - pool_put(&pipex_session_pool, session);
> + if (req->pr_mppe_send.keylenbits <= 0)
> return (EINVAL);
> - }
> pipex_session_init_mppe_send(session,
> req->pr_mppe_send.stateless, req->pr_mppe_send.keylenbits,
> req->pr_mppe_send.master_key);
> @@ -416,56 +410,112 @@ pipex_add_session(struct pipex_session_r
>
> if (pipex_session_is_mppe_required(session)) {
> if (!pipex_session_is_mppe_enabled(session) ||
> - !pipex_session_is_mppe_accepted(session)) {
> - pool_put(&pipex_session_pool, session);
> + !pipex_session_is_mppe_accepted(session))
> return (EINVAL);
> - }
> }
> #endif
>
> - NET_ASSERT_LOCKED();
> + LIST_INSERT_HEAD(&pipex_session_list, session, session_list);
> +
> + /* if first session is added, start timer */
> + if (LIST_NEXT(session, session_list) == NULL)
> + pipex_timer_start();
Adding `session' to the list and starting the timer should be in
link_session(), no? Their counterpart operation are done in
unlink_session().
> +
> + return 0;
> +}
> +
> +void
> +pipex_link_session(struct pipex_session *session)
> +{
> + struct pipex_hash_head *chain;
> +
> + chain = PIPEX_ID_HASHTABLE(session->session_id);
> + LIST_INSERT_HEAD(chain, session, id_chain);
> +#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> + switch (session->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);
> + }
> +#endif
> +
> + session->state = PIPEX_STATE_OPENED;
> +}
> +
> +void
> +pipex_unlink_session(struct pipex_session *session)
> +{
> + if (session->state != PIPEX_STATE_INITIAL) {
> + LIST_REMOVE(session, id_chain);
> +#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
> + }
> +
> + LIST_REMOVE(session, session_list);
> +
> + /* if final session is destroyed, stop timer */
> + if (LIST_EMPTY(&pipex_session_list))
> + pipex_timer_stop();
> +
> + if (session->mppe_recv.old_session_keys)
> + pool_put(&mppe_key_pool, session->mppe_recv.old_session_keys);
> +}
> +
> +Static int
> +pipex_add_session(struct pipex_session_req *req,
> + struct pipex_iface_context *iface)
> +{
> + struct pipex_session *session;
> + struct radix_node *rn;
> + int error;
> +
> + session = pool_get(&pipex_session_pool, PR_WAITOK | PR_ZERO);
> + error = pipex_init_session(session, req, iface);
> + if (error)
> + goto out;
> +
> /* commit the session */
> if (!in_nullhost(session->ip_address.sin_addr)) {
> if (pipex_lookup_by_ip_address(session->ip_address.sin_addr)
> != NULL) {
> - pool_put(&pipex_session_pool, session);
> - return (EADDRINUSE);
> + error = EADDRINUSE;
> + goto unlink;
> }
>
> rn = rn_addroute(&session->ip_address, &session->ip_netmask,
> pipex_rd_head4, session->ps4_rn, RTP_STATIC);
> if (rn == NULL) {
> - pool_put(&pipex_session_pool, session);
> - return (ENOMEM);
> + error = ENOMEM;
> + goto unlink;
> }
> }
> if (0) { /* NOT YET */
> rn = rn_addroute(&session->ip6_address, &session->ip6_prefixlen,
> pipex_rd_head6, session->ps6_rn, RTP_STATIC);
> if (rn == NULL) {
> - pool_put(&pipex_session_pool, session);
> - return (ENOMEM);
> + error = ENOMEM;
> + goto unlink;
> }
> }
>
> - 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);
> - }
> -#endif
> + pipex_link_session(session);
> + pipex_session_log(session, LOG_INFO, "PIPEX is ready.");
>
> - /* if first session is added, start timer */
> - if (LIST_NEXT(session, session_list) == NULL)
> - pipex_timer_start();
> + return 0;
>
> - pipex_session_log(session, LOG_INFO, "PIPEX is ready.");
> +unlink:
> + pipex_unlink_session(session);
> +out:
> + pool_put(&pipex_session_pool, session);
> + return error;
>
> return (0);
> }
> @@ -593,23 +643,7 @@ pipex_destroy_session(struct pipex_sessi
> KASSERT(rn != NULL);
> }
>
> - 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();
> -
> - if (session->mppe_recv.old_session_keys)
> - pool_put(&mppe_key_pool, session->mppe_recv.old_session_keys);
> + pipex_unlink_session(session);
> pool_put(&pipex_session_pool, session);
>
> return (0);
> Index: sys/net/pipex_local.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex_local.h,v
> retrieving revision 1.34
> diff -u -p -r1.34 pipex_local.h
> --- sys/net/pipex_local.h 26 May 2020 07:06:37 -0000 1.34
> +++ sys/net/pipex_local.h 8 Jun 2020 12:07:09 -0000
> @@ -376,6 +376,11 @@ extern struct pipex_hash_head pipex_id_h
>
> void pipex_iface_start (struct pipex_iface_context *);
> void pipex_iface_stop (struct pipex_iface_context *);
> +int pipex_init_session(struct pipex_session *,
> + struct pipex_session_req *,
> + struct pipex_iface_context *);
> +void pipex_link_session(struct pipex_session *);
> +void pipex_unlink_session(struct pipex_session *);
> int pipex_add_session (struct pipex_session_req *, struct
> pipex_iface_context *);
> int pipex_close_session (struct pipex_session_close_req *,
> struct pipex_iface_context *);
