Kernel lock is always taken when we do access to `pxd_pxis' lists and
`pppx_ifs' tree, so rely on it instead of netlock. The search in
`pppx_ifs' tree has no context switch. We also have no context switch
between the `pxi' free unit search and tree insertion.
Use reference counters to make `pxi' dereference safe, instead of
holding net lock. Now pppx_if_find() returns `pxi' with reference
counter bumped, and newly introduced pppx_if_rele() used for release
this `pxi'.
Also, we mark dying `pxi' by setting `pxi_ready' to null, so concurrent
thread can't receive it by pppx_if_find().
I also introduced pppx_if_find_locked() which returns `pxi' but doesn't
bump reference counter. pppx_if_find_locked() and pppx_if_find() both
called with kernel lock held, but I want to keep existing notation where
_locked() function returned data with non bumped counter.
The netlock is still taken when we modify `if_description' of associated
'ifnet', ant it also looks unwanted here, because `if_description' never
used in packet processing path. Unfortunately, this requires to modify
'ifnet' locking, so I want to do this with separate diff.
Index: sys/net/if_pppx.c
===================================================================
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.123
diff -u -p -r1.123 if_pppx.c
--- sys/net/if_pppx.c 19 Nov 2022 15:12:38 -0000 1.123
+++ sys/net/if_pppx.c 21 Nov 2022 00:29:57 -0000
@@ -57,6 +57,7 @@
#include <sys/ioctl.h>
#include <sys/vnode.h>
#include <sys/selinfo.h>
+#include <sys/refcnt.h>
#include <net/if.h>
#include <net/if_types.h>
@@ -132,7 +133,7 @@ struct pppx_dev {
/* queue of packets for userland to service - protected by splnet */
struct mbuf_queue pxd_svcq;
int pxd_waiting; /* [N] */
- LIST_HEAD(,pppx_if) pxd_pxis; /* [N] */
+ LIST_HEAD(,pppx_if) pxd_pxis; /* [K] */
};
LIST_HEAD(, pppx_dev) pppx_devs =
@@ -150,11 +151,12 @@ struct pppx_if_key {
struct pppx_if {
struct pppx_if_key pxi_key; /* [I] must be first
in the struct */
+ struct refcnt pxi_refcnt;
- RBT_ENTRY(pppx_if) pxi_entry; /* [N] */
- LIST_ENTRY(pppx_if) pxi_list; /* [N] */
+ RBT_ENTRY(pppx_if) pxi_entry; /* [K] */
+ LIST_ENTRY(pppx_if) pxi_list; /* [K] */
- int pxi_ready; /* [N] */
+ int pxi_ready; /* [K] */
int pxi_unit; /* [I] */
struct ifnet pxi_if;
@@ -172,7 +174,9 @@ 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);
+static inline void pppx_if_rele(struct pppx_if *);
int pppx_add_session(struct pppx_dev *,
struct pipex_session_req *);
int pppx_del_session(struct pppx_dev *,
@@ -370,11 +374,8 @@ pppxwrite(dev_t dev, struct uio *uio, in
th = mtod(top, struct pppx_hdr *);
m_adj(top, sizeof(struct pppx_hdr));
- NET_LOCK();
-
pxi = pppx_if_find(pxd, th->pppx_id, th->pppx_proto);
if (pxi == NULL) {
- NET_UNLOCK();
m_freem(top);
return (EINVAL);
}
@@ -388,6 +389,8 @@ pppxwrite(dev_t dev, struct uio *uio, in
proto = ntohl(*(uint32_t *)(th + 1));
m_adj(top, sizeof(uint32_t));
+ NET_LOCK();
+
switch (proto) {
case AF_INET:
ipv4_input(&pxi->pxi_if, top);
@@ -405,6 +408,8 @@ pppxwrite(dev_t dev, struct uio *uio, in
NET_UNLOCK();
+ pppx_if_rele(pxi);
+
return (error);
}
@@ -421,17 +426,13 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
break;
case PIPEXDSESSION:
- NET_LOCK();
error = pppx_del_session(pxd,
(struct pipex_session_close_req *)addr);
- NET_UNLOCK();
break;
case PIPEXSIFDESCR:
- NET_LOCK();
error = pppx_set_session_descr(pxd,
(struct pipex_session_descr_req *)addr);
- NET_UNLOCK();
break;
case FIONBIO:
@@ -526,11 +527,10 @@ pppxclose(dev_t dev, int flags, int mode
pxd = pppx_dev_lookup(dev);
- /* XXX */
- NET_LOCK();
- while ((pxi = LIST_FIRST(&pxd->pxd_pxis)))
+ while ((pxi = LIST_FIRST(&pxd->pxd_pxis))) {
+ pxi->pxi_ready = 0;
pppx_if_destroy(pxd, pxi);
- NET_UNLOCK();
+ }
LIST_REMOVE(pxd, pxd_entry);
@@ -566,7 +566,7 @@ 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_key key;
struct pppx_if *pxi;
@@ -582,6 +582,23 @@ pppx_if_find(struct pppx_dev *pxd, int s
return pxi;
}
+static inline struct pppx_if *
+pppx_if_find(struct pppx_dev *pxd, int session_id, int protocol)
+{
+ struct pppx_if *pxi;
+
+ if ((pxi = pppx_if_find_locked(pxd, session_id, protocol)))
+ refcnt_take(&pxi->pxi_refcnt);
+
+ return pxi;
+}
+
+static inline void
+pppx_if_rele(struct pppx_if *pxi)
+{
+ refcnt_rele_wake(&pxi->pxi_refcnt);
+}
+
int
pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
{
@@ -609,7 +626,6 @@ pppx_add_session(struct pppx_dev *pxd, s
pxi->pxi_session = session;
- NET_LOCK();
/* try to set the interface up */
unit = pppx_if_next_unit();
if (unit < 0) {
@@ -617,6 +633,7 @@ pppx_add_session(struct pppx_dev *pxd, s
goto out;
}
+ refcnt_init(&pxi->pxi_refcnt);
pxi->pxi_unit = unit;
pxi->pxi_key.pxik_session_id = req->pr_session_id;
pxi->pxi_key.pxik_protocol = req->pr_protocol;
@@ -627,7 +644,6 @@ pppx_add_session(struct pppx_dev *pxd, s
goto out;
}
LIST_INSERT_HEAD(&pxd->pxd_pxis, pxi, pxi_list);
- NET_UNLOCK();
snprintf(ifp->if_xname, sizeof(ifp->if_xname), "%s%d", "pppx", unit);
ifp->if_mtu = req->pr_peer_mru; /* XXX */
@@ -699,21 +715,18 @@ pppx_add_session(struct pppx_dev *pxd, s
NET_LOCK();
SET(ifp->if_flags, IFF_RUNNING);
- pxi->pxi_ready = 1;
NET_UNLOCK();
+ pxi->pxi_ready = 1;
return (error);
detach:
if_detach(ifp);
- NET_LOCK();
if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
panic("%s: inconsistent RB tree", __func__);
LIST_REMOVE(pxi, pxi_list);
out:
- NET_UNLOCK();
-
pool_put(&pppx_if_pl, pxi);
pipex_rele_session(session);
@@ -725,10 +738,11 @@ pppx_del_session(struct pppx_dev *pxd, s
{
struct pppx_if *pxi;
- pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol);
+ pxi = pppx_if_find_locked(pxd, req->pcr_session_id, req->pcr_protocol);
if (pxi == NULL)
return (EINVAL);
+ pxi->pxi_ready = 0;
pipex_export_session_stats(pxi->pxi_session, &req->pcr_stat);
pppx_if_destroy(pxd, pxi);
return (0);
@@ -744,8 +758,12 @@ pppx_set_session_descr(struct pppx_dev *
if (pxi == NULL)
return (EINVAL);
+ NET_LOCK();
(void)memset(pxi->pxi_if.if_description, 0, IFDESCRSIZE);
strlcpy(pxi->pxi_if.if_description, req->pdr_descr, IFDESCRSIZE);
+ NET_UNLOCK();
+
+ pppx_if_rele(pxi);
return (0);
}
@@ -756,18 +774,17 @@ pppx_if_destroy(struct pppx_dev *pxd, st
struct ifnet *ifp;
struct pipex_session *session;
- NET_ASSERT_LOCKED();
session = pxi->pxi_session;
ifp = &pxi->pxi_if;
- pxi->pxi_ready = 0;
- CLR(ifp->if_flags, IFF_RUNNING);
- pipex_unlink_session(session);
+ refcnt_finalize(&pxi->pxi_refcnt, "pxifinal");
- /* XXXSMP breaks atomicity */
+ NET_LOCK();
+ CLR(ifp->if_flags, IFF_RUNNING);
NET_UNLOCK();
+
+ pipex_unlink_session(session);
if_detach(ifp);
- NET_LOCK();
pipex_rele_session(session);
if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
