Each `struct pppx_if' holds it's own `pipex_session' and this session is
used directly within ifnet's related handlers pppx_if_start() and
pppx_if_output().
pppx_if_destroy() at first destroys `pipex_session' and calls
if_deatch() which can cause context switch. Hypothetically,
pppx_if_start() or pppx_if_output() can receive cpu at this moment and
start to operate with already destroyed session.
I guess the order of `pppx_if' destruction in pppx_if_destroy() is
right. If we call if_detach() before `pipex_session' destruction, after
context switch caused by this if_detach() this session can be accessed
from network stack, but it's own `ifnet' is already destoyed by
if_detach().
Also pppx_add_session() has the same problem: newly created
`pipex_session' can start to operate with half initilaized ifnet, so
ifnet should be initialized before session creation.
Ifnet should be already exist outside corresponding `pipex_session'
life time. And within ifnet's handlers we should be sure that session is
valid.
Diff below drops direct access of this `pipex_session' in ifnet's
handlers.
In pppx_if_start() we obtain corresponding session by
pipex_lookup_by_session_id(). If pppx_if_start() was called in half of
pppx_if_destroy(), pipex_lookup_by_session_id() will return NULL.
Context switch can't be occured in pppx_if_start().
In pppx_if_output() the only ppp_id is requred.
In pppx_add_session() if_attach() moved before `pipex_session' can be
accessed by pipex(4) internals.
Also this will be useful for future. We are going to move out pipex(4)
related code from pppx(4) and after this diff it will be possible just
remove all duplicating code from pppx_if_destroy() and
pppx_add_session().
Index: sys/net/if_pppx.c
===================================================================
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.84
diff -u -p -r1.84 if_pppx.c
--- sys/net/if_pppx.c 18 Apr 2020 04:03:56 -0000 1.84
+++ sys/net/if_pppx.c 14 May 2020 12:47:35 -0000
@@ -157,6 +157,9 @@ struct pppx_if {
struct pppx_dev *pxi_dev;
struct pipex_session pxi_session;
struct pipex_iface_context pxi_ifcontext;
+ int pxi_protocol;
+ int pxi_session_id;
+ int pxi_ppp_id;
};
static inline int
@@ -841,6 +844,9 @@ pppx_add_session(struct pppx_dev *pxd, s
pxi->pxi_key.pxik_session_id = req->pr_session_id;
pxi->pxi_key.pxik_protocol = req->pr_protocol;
pxi->pxi_dev = pxd;
+ pxi->pxi_protocol = req->pr_protocol;
+ pxi->pxi_session_id = req->pr_session_id;
+ pxi->pxi_ppp_id = req->pr_ppp_id;
/* this is safe without splnet since we're not modifying it */
if (RBT_FIND(pppx_ifs, &pppx_ifs, pxi) != NULL) {
@@ -867,6 +873,11 @@ pppx_add_session(struct pppx_dev *pxd, s
ifp->if_softc = pxi;
/* ifp->if_rdomain = req->pr_rdomain; */
+ /* XXXSMP breaks atomicity */
+ NET_UNLOCK();
+ if_attach(ifp);
+ NET_LOCK();
+
/* hook up pipex context */
chain = PIPEX_ID_HASHTABLE(session->session_id);
LIST_INSERT_HEAD(chain, session, id_chain);
@@ -886,11 +897,6 @@ 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);
@@ -1051,25 +1057,34 @@ void
pppx_if_start(struct ifnet *ifp)
{
struct pppx_if *pxi = (struct pppx_if *)ifp->if_softc;
+ struct pipex_session *session;
struct mbuf *m;
int proto;
if (!ISSET(ifp->if_flags, IFF_RUNNING))
return;
+ session = pipex_lookup_by_session_id(pxi->pxi_protocol,
+ pxi->pxi_session_id);
+
for (;;) {
IFQ_DEQUEUE(&ifp->if_snd, m);
if (m == NULL)
break;
+ if (session == NULL) {
+ m_freem(m);
+ continue;
+ }
+
proto = *mtod(m, int *);
m_adj(m, sizeof(proto));
ifp->if_obytes += m->m_pkthdr.len;
ifp->if_opackets++;
- pipex_ppp_output(m, &pxi->pxi_session, proto);
+ pipex_ppp_output(m, session, proto);
}
}
@@ -1129,7 +1144,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_ppp_id;
rw_enter_read(&pppx_devs_lk);
error = mq_enqueue(&pxi->pxi_dev->pxd_svcq, m);
if (error == 0) {
