On Mon, Jun 08, 2020 at 12:49:05PM +0300, Vitaliy Makkoveev wrote:
>
>
> > On 8 Jun 2020, at 11:34, Martin Pieuchot <m...@openbsd.org> wrote:
> >
> > 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.
>
> Except "req->pr_timeout_sec !=0” checking all userland input checks
> were copypasted prom pipex_add_session(). But pppx_add_session()
> inserts pppx(4) related code between checks and session initialization.
> So I decided to keep all pipex(4) related code together for further
> deduplication. Also we want to initialise `ifnet’ before session
> linking.
>
> 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().
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);
+ 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);
+ }
/* 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();
+
+ 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 *);
