Now pipex(4) is fully covered by NET_LOCK() and this is documented. But
we still have an issue with pipex(4) session itself and I guess it's
time to fix it.
We have `pipexinq' and `pipexoutq' mbuf(9) queues to store mbufs. Each
mbuf(9) passed to these queues stores the pointer to corresponding
session referenced as `m_pkthdr.ph_cookie'. We enqueue incoming mbufs for
pppx(4) and incoming and outgoing mbufs for pppac(4). But we don't
enqueue pppoe related mbufs. After packet was enqueued to corresponding
queue we call schednetisr() which just schedules netisr() to run:
---- cut begin ----
780 pipex_ppp_enqueue(struct mbuf *m0, struct pipex_session *session,
781 struct mbuf_queue *mq)
782 {
783 m0->m_pkthdr.ph_cookie = session;
784 /* XXX need to support other protocols */
785 m0->m_pkthdr.ph_ppp_proto = PPP_IP;
786
787 if (mq_enqueue(mq, m0) != 0)
788 return (1);
789
790 schednetisr(NETISR_PIPEX);
791
792 return (0);
793 }
---- cut end ----
Also we have pipex_timer() which should destroy session in safe way, but
it does this only for pppac(4) and only for sessions closed by
`PIPEXDSESSION' command:
---- cut begin ----
812 pipex_timer(void *ignored_arg)
813 {
/* skip */
846 case PIPEX_STATE_CLOSED:
847 /*
848 * mbuf queued in pipexinq or pipexoutq may have a
849 * refererce to this session.
850 */
851 if (!mq_empty(&pipexinq) || !mq_empty(&pipexoutq))
852 continue;
853
854 pipex_destroy_session(session);
855 break;
---- cut end ----
While we destroy sessions through pipex_rele_session() or through
pipex_iface_fini() or through `PIPEXSMODE' command we don't check
`pipexinq' and `pipexoutq' state. This means we can break them.
It's not guaranteed that netisr() will start just after schednetisr()
call. This means we can destroy session, but corresponding mbuf(9) is
stored within `pipexinq' or `pipexoutq'. It's `m_pkthdr.ph_cookie' still
stores pointer to destroyed session and we have use after free issue. I
wonder why we didn't caught panic yet.
I propose to kill `pipexinq', `pipexoutq' and pipexintr(). There is
absolutely no reason them to exist. This should not only fix issue
described above but simplifies code too.
Other ways are to implement reference counters for session or walk
through mbuf(9) queues and kill corresponding mbufs. It doesn't make
sense to go these ways.
Index: lib/libc/sys/sysctl.2
===================================================================
RCS file: /cvs/src/lib/libc/sys/sysctl.2,v
retrieving revision 1.40
diff -u -p -r1.40 sysctl.2
--- lib/libc/sys/sysctl.2 17 May 2020 05:48:39 -0000 1.40
+++ lib/libc/sys/sysctl.2 29 Jul 2020 13:47:40 -0000
@@ -2033,35 +2033,11 @@ The currently defined variable names are
.Bl -column "Third level name" "integer" "Changeable" -offset indent
.It Sy "Third level name" Ta Sy "Type" Ta Sy "Changeable"
.It Dv PIPEXCTL_ENABLE Ta integer Ta yes
-.It Dv PIPEXCTL_INQ Ta node Ta not applicable
-.It Dv PIPEXCTL_OUTQ Ta node Ta not applicable
.El
.Bl -tag -width "123456"
.It Dv PIPEXCTL_ENABLE
If set to 1, enable PIPEX processing.
The default is 0.
-.It Dv PIPEXCTL_INQ Pq Va net.pipex.inq
-Fourth level comprises an array of
-.Vt struct ifqueue
-structures containing information about the PIPEX packet input queue.
-The forth level names for the elements of
-.Vt struct ifqueue
-are the same as described in
-.Li ip.arpq
-in the
-.Dv PF_INET
-section.
-.It Dv PIPEXCTL_OUTQ Pq Va net.pipex.outq
-Fourth level comprises an array of
-.Vt struct ifqueue
-structures containing information about PIPEX packet output queue.
-The forth level names for the elements of
-.Vt struct ifqueue
-are the same as described in
-.Li ip.arpq
-in the
-.Dv PF_INET
-section.
.El
.El
.Ss CTL_VFS
Index: sys/net/if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.616
diff -u -p -r1.616 if.c
--- sys/net/if.c 24 Jul 2020 18:17:14 -0000 1.616
+++ sys/net/if.c 29 Jul 2020 13:47:44 -0000
@@ -909,13 +909,6 @@ if_netisr(void *unused)
KERNEL_UNLOCK();
}
#endif
-#ifdef PIPEX
- if (n & (1 << NETISR_PIPEX)) {
- KERNEL_LOCK();
- pipexintr();
- KERNEL_UNLOCK();
- }
-#endif
t |= n;
}
Index: sys/net/netisr.h
===================================================================
RCS file: /cvs/src/sys/net/netisr.h,v
retrieving revision 1.51
diff -u -p -r1.51 netisr.h
--- sys/net/netisr.h 6 Aug 2019 22:57:54 -0000 1.51
+++ sys/net/netisr.h 29 Jul 2020 13:47:44 -0000
@@ -48,7 +48,6 @@
#define NETISR_IPV6 24 /* same as AF_INET6 */
#define NETISR_ISDN 26 /* same as AF_E164 */
#define NETISR_PPP 28 /* for PPP processing */
-#define NETISR_PIPEX 27 /* for pipex processing */
#define NETISR_BRIDGE 29 /* for bridge processing */
#define NETISR_PPPOE 30 /* for pppoe processing */
#define NETISR_SWITCH 31 /* for switch dataplane */
@@ -68,7 +67,6 @@ void bridgeintr(void);
void pppoeintr(void);
void switchintr(void);
void pfsyncintr(void);
-void pipexintr(void);
#define schednetisr(anisr)
\
do { \
Index: sys/net/pipex.c
===================================================================
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.122
diff -u -p -r1.122 pipex.c
--- sys/net/pipex.c 29 Jul 2020 12:09:31 -0000 1.122
+++ sys/net/pipex.c 29 Jul 2020 13:47:44 -0000
@@ -102,10 +102,6 @@ struct radix_node_head *pipex_rd_head6 =
struct timeout pipex_timer_ch; /* callout timer context */
int pipex_prune = 1; /* [I] walk list every seconds */
-/* pipex traffic queue */
-struct mbuf_queue pipexinq = MBUF_QUEUE_INITIALIZER(IFQ_MAXLEN, IPL_NET);
-struct mbuf_queue pipexoutq = MBUF_QUEUE_INITIALIZER(IFQ_MAXLEN, IPL_NET);
-
/* borrow an mbuf pkthdr field */
#define ph_ppp_proto ether_vtag
@@ -725,82 +721,6 @@ pipex_lookup_by_session_id(int protocol,
}
/***********************************************************************
- * Queue and Software Interrupt Handler
- ***********************************************************************/
-void
-pipexintr(void)
-{
- struct pipex_session *pkt_session;
- u_int16_t proto;
- struct mbuf *m;
- struct mbuf_list ml;
-
- /* ppp output */
- mq_delist(&pipexoutq, &ml);
- while ((m = ml_dequeue(&ml)) != NULL) {
- pkt_session = m->m_pkthdr.ph_cookie;
- if (pkt_session == NULL) {
- m_freem(m);
- continue;
- }
- proto = m->m_pkthdr.ph_ppp_proto;
-
- m->m_pkthdr.ph_cookie = NULL;
- m->m_pkthdr.ph_ppp_proto = 0;
-
- if (pkt_session->is_multicast != 0) {
- struct pipex_session *session;
- struct mbuf *m0;
-
- LIST_FOREACH(session, &pipex_session_list,
- session_list) {
- if (session->pipex_iface !=
- pkt_session->pipex_iface)
- continue;
- if (session->ip_forward == 0 &&
- session->ip6_forward == 0)
- continue;
- m0 = m_copym(m, 0, M_COPYALL, M_NOWAIT);
- if (m0 == NULL) {
- session->stat.oerrors++;
- continue;
- }
- pipex_ppp_output(m0, session, proto);
- }
- m_freem(m);
- } else
- pipex_ppp_output(m, pkt_session, proto);
- }
-
- /* ppp input */
- mq_delist(&pipexinq, &ml);
- while ((m = ml_dequeue(&ml)) != NULL) {
- pkt_session = m->m_pkthdr.ph_cookie;
- if (pkt_session == NULL) {
- m_freem(m);
- continue;
- }
- pipex_ppp_input(m, pkt_session, 0);
- }
-}
-
-Static int
-pipex_ppp_enqueue(struct mbuf *m0, struct pipex_session *session,
- struct mbuf_queue *mq)
-{
- m0->m_pkthdr.ph_cookie = session;
- /* XXX need to support other protocols */
- m0->m_pkthdr.ph_ppp_proto = PPP_IP;
-
- if (mq_enqueue(mq, m0) != 0)
- return (1);
-
- schednetisr(NETISR_PIPEX);
-
- return (0);
-}
-
-/***********************************************************************
* Timer functions
***********************************************************************/
Static void
@@ -852,13 +772,6 @@ pipex_timer(void *ignored_arg)
/* FALLTHROUGH */
case PIPEX_STATE_CLOSED:
- /*
- * mbuf queued in pipexinq or pipexoutq may have a
- * refererce to this session.
- */
- if (!mq_empty(&pipexinq) || !mq_empty(&pipexoutq))
- continue;
-
pipex_destroy_session(session);
break;
@@ -958,12 +871,29 @@ pipex_ip_output(struct mbuf *m0, struct
if (m0 == NULL)
goto dropped;
}
- } else
+
+ pipex_ppp_output(m0, session, PPP_IP);
+ } else {
+ struct pipex_session *session_tmp;
+ struct mbuf *m;
+
m0->m_flags &= ~(M_BCAST|M_MCAST);
- /* output ip packets to the session tunnel */
- if (pipex_ppp_enqueue(m0, session, &pipexoutq))
- goto dropped;
+ LIST_FOREACH(session_tmp, &pipex_session_list, session_list) {
+ if (session_tmp->pipex_iface != session->pipex_iface)
+ continue;
+ if (session_tmp->ip_forward == 0 &&
+ session_tmp->ip6_forward == 0)
+ continue;
+ m = m_copym(m0, 0, M_COPYALL, M_NOWAIT);
+ if (m == NULL) {
+ session_tmp->stat.oerrors++;
+ continue;
+ }
+ pipex_ppp_output(m, session_tmp, PPP_IP);
+ }
+ m_freem(m0);
+ }
return;
drop:
@@ -1241,7 +1171,7 @@ drop:
Static struct mbuf *
pipex_common_input(struct pipex_session *session, struct mbuf *m0, int hlen,
- int plen, int useq)
+ int plen)
{
int proto, ppphlen;
u_char code;
@@ -1289,19 +1219,12 @@ pipex_common_input(struct pipex_session
m_adj(m0, plen - m0->m_pkthdr.len);
}
- if (!useq) {
- pipex_ppp_input(m0, session, 0);
- return (NULL);
- }
+ pipex_ppp_input(m0, session, 0);
+
+ return (NULL);
- /* input ppp packets to kernel session */
- if (pipex_ppp_enqueue(m0, session, &pipexinq) != 0)
- goto dropped;
- else
- return (NULL);
drop:
m_freem(m0);
-dropped:
session->stat.ierrors++;
return (NULL);
@@ -1398,7 +1321,7 @@ pipex_pppoe_input(struct mbuf *m0, struc
sizeof(struct pipex_pppoe_header), (caddr_t)&pppoe);
hlen = sizeof(struct ether_header) + sizeof(struct pipex_pppoe_header);
- if ((m0 = pipex_common_input(session, m0, hlen, ntohs(pppoe.length), 0))
+ if ((m0 = pipex_common_input(session, m0, hlen, ntohs(pppoe.length)))
== NULL)
return (NULL);
m_freem(m0);
@@ -1694,7 +1617,7 @@ pipex_pptp_input(struct mbuf *m0, struct
pipex_pptp_output(NULL, session, 0, 1);
}
- if ((m0 = pipex_common_input(session, m0, hlen, ntohs(gre->len), 1))
+ if ((m0 = pipex_common_input(session, m0, hlen, ntohs(gre->len)))
== NULL) {
/* ok, The packet is for PIPEX */
if (!rewind)
@@ -2129,7 +2052,7 @@ pipex_l2tp_input(struct mbuf *m0, int of
length -= hlen + offset;
hlen += off0 + offset;
- if ((m0 = pipex_common_input(session, m0, hlen, length, 1)) == NULL) {
+ if ((m0 = pipex_common_input(session, m0, hlen, length)) == NULL) {
/* ok, The packet is for PIPEX */
if (!rewind)
session->proto.l2tp.nr_gap += nseq;
@@ -3052,12 +2975,6 @@ pipex_sysctl(int *name, u_int namelen, v
return (ENOTDIR);
return (sysctl_int(oldp, oldlenp, newp, newlen,
&pipex_enable));
- case PIPEXCTL_INQ:
- return (sysctl_mq(name + 1, namelen - 1,
- oldp, oldlenp, newp, newlen, &pipexinq));
- case PIPEXCTL_OUTQ:
- return (sysctl_mq(name + 1, namelen - 1,
- oldp, oldlenp, newp, newlen, &pipexoutq));
default:
return (ENOPROTOOPT);
}
Index: sys/net/pipex.h
===================================================================
RCS file: /cvs/src/sys/net/pipex.h,v
retrieving revision 1.26
diff -u -p -r1.26 pipex.h
--- sys/net/pipex.h 29 Jul 2020 12:09:31 -0000 1.26
+++ sys/net/pipex.h 29 Jul 2020 13:47:44 -0000
@@ -33,15 +33,11 @@
* Names for pipex sysctl objects
*/
#define PIPEXCTL_ENABLE 1
-#define PIPEXCTL_INQ 2
-#define PIPEXCTL_OUTQ 3
-#define PIPEXCTL_MAXID 4
+#define PIPEXCTL_MAXID 2
#define PIPEXCTL_NAMES { \
{ 0, 0 }, \
{ "enable", CTLTYPE_INT }, \
- { "inq", CTLTYPE_NODE }, \
- { "outq", CTLTYPE_NODE }, \
}
#define PIPEX_PROTO_L2TP 1 /* protocol L2TP */
Index: sys/net/pipex_local.h
===================================================================
RCS file: /cvs/src/sys/net/pipex_local.h,v
retrieving revision 1.38
diff -u -p -r1.38 pipex_local.h
--- sys/net/pipex_local.h 29 Jul 2020 12:09:31 -0000 1.38
+++ sys/net/pipex_local.h 29 Jul 2020 13:47:44 -0000
@@ -403,7 +403,8 @@ void pipex_ip_input (st
#ifdef INET6
void pipex_ip6_input (struct mbuf *, struct pipex_session *);
#endif
-struct mbuf *pipex_common_input(struct pipex_session *, struct mbuf
*, int, int, int);
+struct mbuf *pipex_common_input(struct pipex_session *,
+ struct mbuf *, int, int);
#ifdef PIPEX_PPPOE
void pipex_pppoe_output (struct mbuf *, struct pipex_session
*);
