From: dlg Date: Wed, 3 Jun 2015 00:50:09 +0000 (+0000) Subject: there's been a long standing issue in ppp on a tty/serial line where it allocates... X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=fede9e8161cb5ac0652a20b3f658f086b74b255e;p=openbsd there's been a long standing issue in ppp on a tty/serial line where it allocates mbufs at IPL_SOFTTTY, which is above the IPL_NET the mbuf layer protects itself at. recent improvements to diagnostics in pools and mbufs now panic instead of letting these things silently corrupt. this reworks the ppp handling in the tty layer so it has its own private pool to allocate packet memory out of. these packets get built and then queued for softnet to process. softnet dequeues the packet and attaches it to mbufs as external storage before handing it on to the rest of the stack. this was reported on bugs@ and tested by both Walter Daugherity and Martin van den Nieuwelaar ok deraadt@ mpi@ --- diff --git a/sys/net/if_ppp.c b/sys/net/if_ppp.c index f991c8d71fe..89f4482c177 100644 --- a/sys/net/if_ppp.c +++ b/sys/net/if_ppp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_ppp.c,v 1.83 2015/05/13 10:42:46 jsg Exp $ */ +/* $OpenBSD: if_ppp.c,v 1.84 2015/06/03 00:50:09 dlg Exp $ */ /* $NetBSD: if_ppp.c,v 1.39 1997/05/17 21:11:59 christos Exp $ */ /* @@ -154,18 +154,10 @@ static void ppp_ifstart(struct ifnet *ifp); int ppp_clone_create(struct if_clone *, int); int ppp_clone_destroy(struct ifnet *); -/* - * Some useful mbuf macros not in mbuf.h. - */ -#define M_IS_CLUSTER(m) ((m)->m_flags & M_EXT) - -#define M_DATASTART(m) \ - (M_IS_CLUSTER(m) ? (m)->m_ext.ext_buf : \ - (m)->m_flags & M_PKTHDR ? (m)->m_pktdat : (m)->m_dat) - -#define M_DATASIZE(m) \ - (M_IS_CLUSTER(m) ? (m)->m_ext.ext_size : \ - (m)->m_flags & M_PKTHDR ? MHLEN: MLEN) +void ppp_pkt_list_init(struct ppp_pkt_list *, u_int); +int ppp_pkt_enqueue(struct ppp_pkt_list *, struct ppp_pkt *); +struct ppp_pkt *ppp_pkt_dequeue(struct ppp_pkt_list *); +struct mbuf * ppp_pkt_mbuf(struct ppp_pkt *); /* * We steal two bits in the mbuf m_flags, to mark high-priority packets @@ -234,7 +226,7 @@ ppp_clone_create(struct if_clone *ifc, int unit) IFQ_SET_MAXLEN(&sc->sc_if.if_snd, IFQ_MAXLEN); mq_init(&sc->sc_inq, IFQ_MAXLEN, IPL_NET); IFQ_SET_MAXLEN(&sc->sc_fastq, IFQ_MAXLEN); - IFQ_SET_MAXLEN(&sc->sc_rawq, IFQ_MAXLEN); + ppp_pkt_list_init(&sc->sc_rawq, IFQ_MAXLEN); IFQ_SET_READY(&sc->sc_if.if_snd); if_attach(&sc->sc_if); if_alloc_sadl(&sc->sc_if); @@ -315,6 +307,7 @@ pppalloc(pid_t pid) void pppdealloc(struct ppp_softc *sc) { + struct ppp_pkt *pkt; struct mbuf *m; splsoftassert(IPL_SOFTNET); @@ -323,12 +316,8 @@ pppdealloc(struct ppp_softc *sc) sc->sc_if.if_flags &= ~(IFF_UP|IFF_RUNNING); sc->sc_devp = NULL; sc->sc_xfer = 0; - for (;;) { - IF_DEQUEUE(&sc->sc_rawq, m); - if (m == NULL) - break; - m_freem(m); - } + while ((pkt = ppp_pkt_dequeue(&sc->sc_rawq)) != NULL) + ppp_pkt_free(pkt); while ((m = mq_dequeue(&sc->sc_inq)) != NULL) m_freem(m); for (;;) { @@ -1052,31 +1041,28 @@ void pppintr(void) { struct ppp_softc *sc; - int s, s2; + int s; + struct ppp_pkt *pkt; struct mbuf *m; splsoftassert(IPL_SOFTNET); - s = splsoftnet(); /* XXX - what's the point of this? see comment above */ LIST_FOREACH(sc, &ppp_softc_list, sc_list) { if (!(sc->sc_flags & SC_TBUSY) && (!IFQ_IS_EMPTY(&sc->sc_if.if_snd) || !IFQ_IS_EMPTY(&sc->sc_fastq))) { - s2 = splnet(); + s = splnet(); sc->sc_flags |= SC_TBUSY; - splx(s2); + splx(s); (*sc->sc_start)(sc); } - while (!IFQ_IS_EMPTY(&sc->sc_rawq)) { - s2 = splnet(); - IF_DEQUEUE(&sc->sc_rawq, m); - splx(s2); + while ((pkt = ppp_pkt_dequeue(&sc->sc_rawq)) != NULL) { + m = ppp_pkt_mbuf(pkt); if (m == NULL) - break; + continue; ppp_inproc(sc, m); } } - splx(s); } #ifdef PPP_COMPRESS @@ -1199,15 +1185,11 @@ ppp_ccp_closed(struct ppp_softc *sc) * were omitted. */ void -ppppktin(struct ppp_softc *sc, struct mbuf *m, int lost) +ppppktin(struct ppp_softc *sc, struct ppp_pkt *pkt, int lost) { - int s = splnet(); - - if (lost) - m->m_flags |= M_ERRMARK; - IF_ENQUEUE(&sc->sc_rawq, m); - schednetisr(NETISR_PPP); - splx(s); + pkt->p_hdr.ph_errmark = lost; + if (ppp_pkt_enqueue(&sc->sc_rawq, pkt) == 0) + schednetisr(NETISR_PPP); } /* @@ -1389,19 +1371,6 @@ ppp_inproc(struct ppp_softc *sc, struct mbuf *m) } #endif /* VJC */ - /* - * If the packet will fit in a header mbuf, don't waste a - * whole cluster on it. - */ - if (ilen <= MHLEN && M_IS_CLUSTER(m)) { - MGETHDR(mp, M_DONTWAIT, MT_DATA); - if (mp != NULL) { - m_copydata(m, 0, ilen, mtod(mp, caddr_t)); - m_freem(m); - m = mp; - m->m_len = ilen; - } - } m->m_pkthdr.len = ilen; m->m_pkthdr.rcvif = ifp; @@ -1542,4 +1511,96 @@ ppp_ifstart(struct ifnet *ifp) sc = ifp->if_softc; (*sc->sc_start)(sc); } + +void +ppp_pkt_list_init(struct ppp_pkt_list *pl, u_int limit) +{ + mtx_init(&pl->pl_mtx, IPL_TTY); + pl->pl_head = pl->pl_tail = NULL; + pl->pl_count = 0; + pl->pl_limit = limit; +} + +int +ppp_pkt_enqueue(struct ppp_pkt_list *pl, struct ppp_pkt *pkt) +{ + int drop = 0; + + mtx_enter(&pl->pl_mtx); + if (pl->pl_count < pl->pl_limit) { + if (pl->pl_tail == NULL) + pl->pl_head = pl->pl_tail = pkt; + else { + PKT_NEXTPKT(pl->pl_tail) = pkt; + pl->pl_tail = pkt; + } + PKT_NEXTPKT(pkt) = NULL; + pl->pl_count++; + } else + drop = 1; + mtx_leave(&pl->pl_mtx); + + if (drop) + ppp_pkt_free(pkt); + + return (drop); +} + +struct ppp_pkt * +ppp_pkt_dequeue(struct ppp_pkt_list *pl) +{ + struct ppp_pkt *pkt; + + mtx_enter(&pl->pl_mtx); + pkt = pl->pl_head; + if (pkt != NULL) { + pl->pl_head = PKT_NEXTPKT(pkt); + if (pl->pl_head == NULL) + pl->pl_tail = NULL; + + pl->pl_count--; + } + mtx_leave(&pl->pl_mtx); + + return (pkt); +} + +struct mbuf * +ppp_pkt_mbuf(struct ppp_pkt *pkt0) +{ + extern struct pool ppp_pkts; + struct mbuf *m0 = NULL, **mp = &m0, *m; + struct ppp_pkt *pkt = pkt0; + size_t len = 0; + + do { + MGETHDR(m, M_DONTWAIT, MT_DATA); + if (m == NULL) + goto fail; + + MEXTADD(m, pkt, sizeof(*pkt), M_EXTWR, + m_extfree_pool, &ppp_pkts); + m->m_data += sizeof(pkt->p_hdr); + m->m_len = PKT_LEN(pkt); + + len += m->m_len; + + *mp = m; + mp = &m->m_next; + + pkt = PKT_NEXT(pkt); + } while (pkt != NULL); + + m0->m_pkthdr.len = len; + if (pkt0->p_hdr.ph_errmark) + m0->m_flags |= M_ERRMARK; + + return (m0); + +fail: + m_freem(m0); + ppp_pkt_free(pkt0); + return (NULL); +} + #endif /* NPPP > 0 */ diff --git a/sys/net/if_pppvar.h b/sys/net/if_pppvar.h index 130fc7a07d3..9735b48b3bb 100644 --- a/sys/net/if_pppvar.h +++ b/sys/net/if_pppvar.h @@ -1,4 +1,4 @@ -/* $OpenBSD: if_pppvar.h,v 1.16 2015/04/10 13:58:20 dlg Exp $ */ +/* $OpenBSD: if_pppvar.h,v 1.17 2015/06/03 00:50:09 dlg Exp $ */ /* $NetBSD: if_pppvar.h,v 1.5 1997/01/03 07:23:29 mikel Exp $ */ /* * if_pppvar.h - private structures and declarations for PPP. @@ -83,6 +83,16 @@ #define NP_IP 0 /* Internet Protocol */ #define NUM_NP 1 /* Number of NPs. */ +struct ppp_pkt; + +struct ppp_pkt_list { + struct mutex pl_mtx; + struct ppp_pkt *pl_head; + struct ppp_pkt *pl_tail; + u_int pl_count; + u_int pl_limit; +}; + /* * Structure describing each ppp unit. */ @@ -97,7 +107,7 @@ struct ppp_softc { void (*sc_relinq)(struct ppp_softc *); /* relinquish ifunit */ u_int16_t sc_mru; /* max receive unit */ pid_t sc_xfer; /* used in transferring unit */ - struct ifqueue sc_rawq; /* received packets */ + struct ppp_pkt_list sc_rawq; /* received packets */ struct mbuf_queue sc_inq; /* queue of input packets for daemon */ struct ifqueue sc_fastq; /* interactive output packet q */ struct mbuf *sc_togo; /* output packet ready to go */ @@ -122,9 +132,9 @@ struct ppp_softc { ext_accm sc_asyncmap; /* async control character map */ u_int32_t sc_rasyncmap; /* receive async control char map */ struct mbuf *sc_outm; /* mbuf chain currently being output */ - struct mbuf *sc_m; /* pointer to input mbuf chain */ - struct mbuf *sc_mc; /* pointer to current input mbuf */ - char *sc_mp; /* ptr to next char in input mbuf */ + struct ppp_pkt *sc_pkt; /* pointer to input pkt chain */ + struct ppp_pkt *sc_pktc; /* pointer to current input pkt */ + uint8_t *sc_pktp; /* ptr to next char in input pkt */ u_int16_t sc_ilen; /* length of input packet so far */ u_int16_t sc_fcs; /* FCS so far (input) */ u_int16_t sc_outfcs; /* FCS so far for output packet */ @@ -134,13 +144,33 @@ struct ppp_softc { }; #ifdef _KERNEL + +struct ppp_pkt_hdr { + struct ppp_pkt *ph_next; /* next in pkt chain */ + struct ppp_pkt *ph_pkt; /* prev in chain or next in list */ + uint16_t ph_len; + uint16_t ph_errmark; +}; + +struct ppp_pkt { + struct ppp_pkt_hdr p_hdr; + uint8_t p_buf[MCLBYTES - sizeof(struct ppp_pkt_hdr)]; +}; + +void ppp_pkt_free(struct ppp_pkt *); + +#define PKT_NEXT(_p) ((_p)->p_hdr.ph_next) +#define PKT_PREV(_p) ((_p)->p_hdr.ph_pkt) +#define PKT_NEXTPKT(_p) ((_p)->p_hdr.ph_pkt) +#define PKT_LEN(_p) ((_p)->p_hdr.ph_len) + extern struct ppp_softc ppp_softc[]; struct ppp_softc *pppalloc(pid_t pid); void pppdealloc(struct ppp_softc *sc); int pppioctl(struct ppp_softc *sc, u_long cmd, caddr_t data, int flag, struct proc *p); -void ppppktin(struct ppp_softc *sc, struct mbuf *m, int lost); +void ppppktin(struct ppp_softc *sc, struct ppp_pkt *pkt, int lost); struct mbuf *ppp_dequeue(struct ppp_softc *sc); void ppp_restart(struct ppp_softc *sc); int pppoutput(struct ifnet *, struct mbuf *, diff --git a/sys/net/ppp_tty.c b/sys/net/ppp_tty.c index e2cc9fd5178..590b531e372 100644 --- a/sys/net/ppp_tty.c +++ b/sys/net/ppp_tty.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ppp_tty.c,v 1.32 2015/04/10 13:58:20 dlg Exp $ */ +/* $OpenBSD: ppp_tty.c,v 1.33 2015/06/03 00:50:09 dlg Exp $ */ /* $NetBSD: ppp_tty.c,v 1.12 1997/03/24 21:23:10 christos Exp $ */ /* @@ -110,6 +110,8 @@ #include #include #include +#include +#include #include #include @@ -140,22 +142,14 @@ void pppasyncstart(struct ppp_softc *); void pppasyncctlp(struct ppp_softc *); void pppasyncrelinq(struct ppp_softc *); void ppp_timeout(void *); -void pppgetm(struct ppp_softc *sc); +void ppppkt(struct ppp_softc *sc); void pppdumpb(u_char *b, int l); void ppplogchar(struct ppp_softc *, int); -/* - * Some useful mbuf macros not in mbuf.h. - */ -#define M_IS_CLUSTER(m) ((m)->m_flags & M_EXT) +struct rwlock ppp_pkt_init = RWLOCK_INITIALIZER("ppppktini"); +struct pool ppp_pkts; -#define M_DATASTART(m) \ - (M_IS_CLUSTER(m) ? (m)->m_ext.ext_buf : \ - (m)->m_flags & M_PKTHDR ? (m)->m_pktdat : (m)->m_dat) - -#define M_DATASIZE(m) \ - (M_IS_CLUSTER(m) ? (m)->m_ext.ext_size : \ - (m)->m_flags & M_PKTHDR ? MHLEN: MLEN) +#define PKT_MAXLEN(_sc) ((_sc)->sc_mru + PPP_HDRLEN + PPP_FCSLEN) /* * Does c need to be escaped? @@ -188,6 +182,16 @@ pppopen(dev_t dev, struct tty *tp) if ((error = suser(p, 0)) != 0) return (error); + rw_enter_write(&ppp_pkt_init); + if (ppp_pkts.pr_size == 0) { + extern struct kmem_pa_mode kp_dma_contig; + + pool_init(&ppp_pkts, sizeof(struct ppp_pkt), 0, 0, 0, "ppppkts", NULL); + pool_setipl(&ppp_pkts, IPL_TTY); /* IPL_SOFTTTY */ + pool_set_constraints(&ppp_pkts, &kp_dma_contig); + } + rw_exit_write(&ppp_pkt_init); + s = spltty(); if (tp->t_line == PPPDISC) { @@ -208,7 +212,7 @@ pppopen(dev_t dev, struct tty *tp) timeout_set(&sc->sc_timo, ppp_timeout, sc); sc->sc_ilen = 0; - sc->sc_m = NULL; + sc->sc_pkt = NULL; bzero(sc->sc_asyncmap, sizeof(sc->sc_asyncmap)); sc->sc_asyncmap[0] = 0xffffffff; sc->sc_asyncmap[3] = 0x60000000; @@ -218,7 +222,7 @@ pppopen(dev_t dev, struct tty *tp) sc->sc_ctlp = pppasyncctlp; sc->sc_relinq = pppasyncrelinq; sc->sc_outm = NULL; - pppgetm(sc); + ppppkt(sc); sc->sc_if.if_flags |= IFF_RUNNING; sc->sc_if.if_baudrate = tp->t_ospeed; @@ -269,9 +273,9 @@ pppasyncrelinq(struct ppp_softc *sc) m_freem(sc->sc_outm); sc->sc_outm = NULL; } - if (sc->sc_m) { - m_freem(sc->sc_m); - sc->sc_m = NULL; + if (sc->sc_pkt != NULL) { + ppp_pkt_free(sc->sc_pkt); + sc->sc_pkt = sc->sc_pktc = NULL; } if (sc->sc_flags & SC_TIMEOUT) { timeout_del(&sc->sc_timo); @@ -436,7 +440,7 @@ ppptioctl(struct tty *tp, u_long cmd, caddr_t data, int flag, struct proc *p) default: error = pppioctl(sc, cmd, data, flag, p); if (error == 0 && cmd == PPPIOCSMRU) - pppgetm(sc); + ppppkt(sc); } return error; @@ -743,28 +747,42 @@ ppp_timeout(void *x) * Allocate enough mbuf to handle current MRU. */ void -pppgetm(struct ppp_softc *sc) +ppppkt(struct ppp_softc *sc) { - struct mbuf *m, **mp; + struct ppp_pkt **pktp, *pkt; int len; int s; s = spltty(); - mp = &sc->sc_m; - for (len = sc->sc_mru + PPP_HDRLEN + PPP_FCSLEN; len > 0; ){ - if ((m = *mp) == NULL) { - MGETHDR(m, M_DONTWAIT, MT_DATA); - if (m == NULL) + pktp = &sc->sc_pkt; + for (len = PKT_MAXLEN(sc); len > 0; len -= sizeof(pkt->p_buf)) { + pkt = *pktp; + if (pkt == NULL) { + pkt = pool_get(&ppp_pkts, PR_NOWAIT); + if (pkt == NULL) break; - *mp = m; - MCLGET(m, M_DONTWAIT); + PKT_NEXT(pkt) = NULL; + PKT_PREV(pkt) = *pktp; + PKT_LEN(pkt) = 0; + *pktp = pkt; } - len -= M_DATASIZE(m); - mp = &m->m_next; + pktp = &PKT_NEXT(pkt); } splx(s); } +void +ppp_pkt_free(struct ppp_pkt *pkt) +{ + struct ppp_pkt *next; + + while (pkt != NULL) { + next = PKT_NEXT(pkt); + pool_put(&ppp_pkts, pkt); + pkt = next; + } +} + /* * tty interface receiver interrupt. */ @@ -777,7 +795,7 @@ int pppinput(int c, struct tty *tp) { struct ppp_softc *sc; - struct mbuf *m; + struct ppp_pkt *pkt; int ilen, s; sc = (struct ppp_softc *) tp->t_sc; @@ -870,29 +888,29 @@ pppinput(int c, struct tty *tp) } /* - * Remove FCS trailer. Somewhat painful... + * Remove FCS trailer. */ ilen -= 2; - if (--sc->sc_mc->m_len == 0) { - for (m = sc->sc_m; m->m_next != sc->sc_mc; m = m->m_next) - ; - sc->sc_mc = m; + pkt = sc->sc_pktc; + if (--PKT_LEN(pkt) == 0) { + pkt = PKT_PREV(pkt); + sc->sc_pktc = pkt; } - sc->sc_mc->m_len--; + PKT_LEN(pkt)--; /* excise this mbuf chain */ - m = sc->sc_m; - sc->sc_m = sc->sc_mc->m_next; - sc->sc_mc->m_next = NULL; + pkt = sc->sc_pkt; + sc->sc_pkt = sc->sc_pktc = PKT_NEXT(sc->sc_pktc); + PKT_NEXT(pkt) = NULL; - ppppktin(sc, m, sc->sc_flags & SC_PKTLOST); + ppppktin(sc, pkt, sc->sc_flags & SC_PKTLOST); if (sc->sc_flags & SC_PKTLOST) { s = spltty(); sc->sc_flags &= ~SC_PKTLOST; splx(s); } - pppgetm(sc); + ppppkt(sc); return 0; } @@ -927,19 +945,18 @@ pppinput(int c, struct tty *tp) */ if (sc->sc_ilen == 0) { /* reset the first input mbuf */ - if (sc->sc_m == NULL) { - pppgetm(sc); - if (sc->sc_m == NULL) { + if (sc->sc_pkt == NULL) { + ppppkt(sc); + if (sc->sc_pkt == NULL) { if (sc->sc_flags & SC_DEBUG) printf("%s: no input mbufs!\n", sc->sc_if.if_xname); goto flush; } } - m = sc->sc_m; - m->m_len = 0; - m->m_data = M_DATASTART(sc->sc_m); - sc->sc_mc = m; - sc->sc_mp = mtod(m, char *); + pkt = sc->sc_pkt; + PKT_LEN(pkt) = 0; + sc->sc_pktc = pkt; + sc->sc_pktp = pkt->p_buf; sc->sc_fcs = PPP_INITFCS; if (c != PPP_ALLSTATIONS) { if (sc->sc_flags & SC_REJ_COMP_AC) { @@ -948,10 +965,10 @@ pppinput(int c, struct tty *tp) sc->sc_if.if_xname, c); goto flush; } - *sc->sc_mp++ = PPP_ALLSTATIONS; - *sc->sc_mp++ = PPP_UI; + *sc->sc_pktp++ = PPP_ALLSTATIONS; + *sc->sc_pktp++ = PPP_UI; sc->sc_ilen += 2; - m->m_len += 2; + PKT_LEN(pkt) += 2; } } if (sc->sc_ilen == 1 && c != PPP_UI) { @@ -962,43 +979,42 @@ pppinput(int c, struct tty *tp) } if (sc->sc_ilen == 2 && (c & 1) == 1) { /* a compressed protocol */ - *sc->sc_mp++ = 0; + *sc->sc_pktp++ = 0; sc->sc_ilen++; - sc->sc_mc->m_len++; + PKT_LEN(sc->sc_pktc)++; } if (sc->sc_ilen == 3 && (c & 1) == 0) { if (sc->sc_flags & SC_DEBUG) printf("%s: bad protocol %x\n", sc->sc_if.if_xname, - (sc->sc_mp[-1] << 8) + c); + (sc->sc_pktp[-1] << 8) + c); goto flush; } /* packet beyond configured mru? */ - if (++sc->sc_ilen > sc->sc_mru + PPP_HDRLEN + PPP_FCSLEN) { + if (++sc->sc_ilen > PKT_MAXLEN(sc)) { if (sc->sc_flags & SC_DEBUG) printf("%s: packet too big\n", sc->sc_if.if_xname); goto flush; } - /* is this mbuf full? */ - m = sc->sc_mc; - if (M_TRAILINGSPACE(m) <= 0) { - if (m->m_next == NULL) { - pppgetm(sc); - if (m->m_next == NULL) { + /* is this packet full? */ + pkt = sc->sc_pktc; + if (PKT_LEN(pkt) >= sizeof(pkt->p_buf)) { + if (PKT_NEXT(pkt) == NULL) { + ppppkt(sc); + if (PKT_NEXT(pkt) == NULL) { if (sc->sc_flags & SC_DEBUG) - printf("%s: too few input mbufs!\n", sc->sc_if.if_xname); + printf("%s: too few input packets!\n", sc->sc_if.if_xname); goto flush; } } - sc->sc_mc = m = m->m_next; - m->m_len = 0; - m->m_data = M_DATASTART(m); - sc->sc_mp = mtod(m, char *); + sc->sc_pktc = pkt = PKT_NEXT(pkt); + PKT_LEN(pkt) = 0; + sc->sc_pktp = pkt->p_buf; } - ++m->m_len; - *sc->sc_mp++ = c; + ++PKT_LEN(pkt); + *sc->sc_pktp++ = c; sc->sc_fcs = PPP_FCS(sc->sc_fcs, c); return 0;