On 29/07/20(Wed) 23:04, Vitaliy Makkoveev wrote:
> 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.
Time is fine. Make sure you watch for possible fallouts. If you're
curious you can generate Flamegraphs or profile the kernel with and
without this diff to get an idea of what get executed. This change
should improve latency by reducing contention on the KERNEL_LOCK().
ok mpi@
Note that as a next step it could be beneficial to pass an `ifp' pointer
to pipex_ip{,6}_input() and maybe even pipex_ppp_input() to reduce the
number of if_get(9). This will make an interesting pattern appear ;)
> 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 *);
