As Claudio Jeker noticed, NET_LOCK() can release KERNEL_LOCK(). pppx(4)
code has some NET_LOCK() dances which make it unsafe. Concurent thread
can receive CPU and enter to pppx_if_destroy() while we dance with
NET_LOCK(). The idea is to deny access to pxi at destruction stage.
If pxi_if is removed from pppx_ifs tree under pppx_ifs_lk nobody will
access to it after pppx_ifs_lk release. Already used pppx_if can't be
accessed by pppx_del_session() because all this code is under
NET_LOCK().
I don't want to do all pxi_if destruction under pppx_ifs_lk. So new
function pppx_if_find_locked() introduced. It does the same what
pppx_if_find() does but pppx_ifs_lk should be held.
So grab the lock, find and remove pxi_if from tree and list, realease
lock, kill pppx_if.
I wish to add refconters to pppx_if in future to protect it the tun(4)
style. This diff will be the first step in this direction.
Index: sys/net/if_pppx.c
===================================================================
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.81
diff -u -p -r1.81 if_pppx.c
--- sys/net/if_pppx.c 7 Apr 2020 07:11:22 -0000 1.81
+++ sys/net/if_pppx.c 7 Apr 2020 13:41:07 -0000
@@ -170,7 +170,8 @@ RBT_HEAD(pppx_ifs, pppx_if) pppx_ifs = R
RBT_PROTOTYPE(pppx_ifs, pppx_if, pxi_entry, pppx_if_cmp);
int pppx_if_next_unit(void);
-struct pppx_if *pppx_if_find(struct pppx_dev *, int, int);
+struct pppx_if *pppx_if_find_locked(struct pppx_dev *, int, int);
+static inline struct pppx_if *pppx_if_find(struct pppx_dev *, int, int);
int pppx_add_session(struct pppx_dev *,
struct pipex_session_req *);
int pppx_del_session(struct pppx_dev *,
@@ -594,8 +595,19 @@ pppxclose(dev_t dev, int flags, int mode
/* XXX */
NET_LOCK();
- while ((pxi = LIST_FIRST(&pxd->pxd_pxis)))
+ rw_enter_write(&pppx_ifs_lk);
+ while ((pxi = LIST_FIRST(&pxd->pxd_pxis))) {
+ 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);
+
pppx_if_destroy(pxd, pxi);
+
+ rw_enter_write(&pppx_ifs_lk);
+ }
+ rw_exit_write(&pppx_ifs_lk);
NET_UNLOCK();
LIST_REMOVE(pxd, pxd_entry);
@@ -641,24 +653,37 @@ pppx_if_next_unit(void)
}
struct pppx_if *
-pppx_if_find(struct pppx_dev *pxd, int session_id, int protocol)
+pppx_if_find_locked(struct pppx_dev *pxd, int session_id, int protocol)
{
struct pppx_if *s, *p;
+
+ rw_assert_anylock(&pppx_ifs_lk);
+
s = malloc(sizeof(*s), M_DEVBUF, M_WAITOK | M_ZERO);
s->pxi_key.pxik_session_id = session_id;
s->pxi_key.pxik_protocol = protocol;
- rw_enter_read(&pppx_ifs_lk);
p = RBT_FIND(pppx_ifs, &pppx_ifs, s);
if (p && p->pxi_ready == 0)
p = NULL;
- rw_exit_read(&pppx_ifs_lk);
free(s, M_DEVBUF, sizeof(*s));
return (p);
}
+static inline struct pppx_if *
+pppx_if_find(struct pppx_dev *pxd, int session_id, int protocol)
+{
+ struct pppx_if *pxi;
+
+ rw_enter_read(&pppx_ifs_lk);
+ pxi = pppx_if_find_locked(pxd, session_id, protocol);
+ rw_exit_read(&pppx_ifs_lk);
+
+ return pxi;
+}
+
int
pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
{
@@ -948,14 +973,22 @@ pppx_del_session(struct pppx_dev *pxd, s
{
struct pppx_if *pxi;
- pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol);
+ rw_enter_write(&pppx_ifs_lk);
+ pxi = pppx_if_find_locked(pxd, req->pcr_session_id,
+ req->pcr_protocol);
if (pxi == NULL)
return (EINVAL);
+ 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);
+
req->pcr_stat = pxi->pxi_session.stat;
pppx_if_destroy(pxd, pxi);
- return (0);
+
+ return 0;
}
int
@@ -1037,12 +1070,6 @@ pppx_if_destroy(struct pppx_dev *pxd, st
NET_UNLOCK();
if_detach(ifp);
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__);
- LIST_REMOVE(pxi, pxi_list);
- rw_exit_write(&pppx_ifs_lk);
pool_put(pppx_if_pl, pxi);
}
