Subject: Re: pppoe & mbuf chain
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
From: Jun-ichiro itojun Hagino <itojun@iijlab.net>
List: tech-net
Date: 06/22/2002 14:40:42
>> that have high possibility of failure (if m->m_pkthdr.len > MHLEN,
>> m_pullup will fail), so i'd suggest rewriting pppoe code not to require
>> "single mbuf" restriction.
>ETHERTYPE_PPPOEDISC packets can be so large actually?
there are variable-length header present so i guess it safer not to
assume maximum length. in fact, if_pppoe.c do try to allocate cluster
mbuf for outgoing PPPOEDISC packet.
itojun@no test environment
Index: if_pppoe.c
===================================================================
RCS file: /cvsroot/syssrc/sys/net/if_pppoe.c,v
retrieving revision 1.25
diff -u -r1.25 if_pppoe.c
--- if_pppoe.c 2002/06/22 05:33:42 1.25
+++ if_pppoe.c 2002/06/22 05:40:54
@@ -67,7 +67,19 @@
#undef PPPOE_DEBUG /* XXX - remove this or make it an option */
/* #define PPPOE_DEBUG 1 */
-#define PPPOE_HEADERLEN 6
+struct pppoehdr {
+ u_int8_t vertype;
+ u_int8_t code;
+ u_int16_t session;
+ u_int16_t plen;
+} __attribute__((__packed__));
+
+struct pppoetag {
+ u_int16_t tag;
+ u_int16_t len;
+} __attribute__((__packed__));
+
+#define PPPOE_HEADERLEN sizeof(struct pppoehdr)
#define PPPOE_VERTYPE 0x11 /* VER=1, TYPE = 1 */
#define PPPOE_TAG_EOL 0x0000 /* end of list */
@@ -90,11 +102,6 @@
/* two byte PPP protocol discriminator, then IP data */
#define PPPOE_MAXMTU (ETHERMTU-PPPOE_HEADERLEN-2)
-/* Read a 16 bit unsigned value from a buffer */
-#define PPPOE_READ_16(PTR, VAL) \
- (VAL) = ((PTR)[0] << 8) | (PTR)[1]; \
- (PTR)+=2
-
/* Add a 16 bit unsigned value to a buffer pointed to by PTR */
#define PPPOE_ADD_16(PTR, VAL) \
*(PTR)++ = (VAL) / 256; \
@@ -147,11 +154,11 @@
/* input routines */
static void pppoe_input(void);
static void pppoe_disc_input(struct mbuf *);
-static void pppoe_dispatch_disc_pkt(u_int8_t *, size_t, struct ifnet *, struct ether_header *);
-static void pppoe_data_input(struct mbuf *m);
+static void pppoe_dispatch_disc_pkt(struct mbuf *, int);
+static void pppoe_data_input(struct mbuf *);
/* management routines */
-void pppoeattach(int count);
+void pppoeattach(int);
static int pppoe_connect(struct pppoe_softc *);
static int pppoe_disconnect(struct pppoe_softc *);
static void pppoe_abort_connect(struct pppoe_softc *);
@@ -161,7 +168,7 @@
static void pppoe_start(struct ifnet *);
/* internal timeout handling */
-static void pppoe_timeout(void*);
+static void pppoe_timeout(void *);
/* sending actual protocol controll packets */
static int pppoe_send_padi(struct pppoe_softc *);
@@ -169,7 +176,7 @@
static int pppoe_send_padt(struct pppoe_softc *);
/* raw output */
-static int pppoe_output(struct pppoe_softc *sc, struct mbuf *m);
+static int pppoe_output(struct pppoe_softc *, struct mbuf *);
/* internal helper functions */
static struct pppoe_softc * pppoe_find_softc_by_session(u_int, struct ifnet *);
@@ -268,7 +275,8 @@
{
struct pppoe_softc *sc;
- if (session == 0) return NULL;
+ if (session == 0)
+ return NULL;
LIST_FOREACH(sc, &pppoe_softc_list, sc_list) {
if (sc->sc_state == PPPOE_STATE_SESSION
@@ -289,9 +297,11 @@
{
struct pppoe_softc *sc, *t;
- if (LIST_EMPTY(&pppoe_softc_list)) return NULL;
+ if (LIST_EMPTY(&pppoe_softc_list))
+ return NULL;
- if (len != sizeof sc) return NULL;
+ if (len != sizeof sc)
+ return NULL;
memcpy(&t, token, len);
LIST_FOREACH(sc, &pppoe_softc_list, sc_list)
@@ -365,63 +375,94 @@
}
/* analyze and handle a single received packet while not in session state */
-static void pppoe_dispatch_disc_pkt(u_int8_t *p, size_t size, struct ifnet *rcvif, struct ether_header *eh)
+static void pppoe_dispatch_disc_pkt(struct mbuf *m, int off)
{
u_int16_t tag, len;
- u_int8_t vertype, code;
u_int16_t session, plen;
struct pppoe_softc *sc;
const char *err_msg = NULL;
- u_int8_t * ac_cookie;
+ u_int8_t *ac_cookie;
size_t ac_cookie_len;
+ struct pppoehdr *ph;
+ struct pppoetag *pt;
+ struct mbuf *n;
+ int noff;
+ struct ether_header eh;
+ m_copydata(m, 0, sizeof(eh), (caddr_t)&eh);
+ m_adj(m, sizeof eh);
+
ac_cookie = NULL;
ac_cookie_len = 0;
session = 0;
- if (size <= PPPOE_HEADERLEN) {
- printf("pppoe: packet too short: %ld\n", (long)size);
- return;
- }
- vertype = *p++;
- if (vertype != PPPOE_VERTYPE) {
- printf("pppoe: unknown version/type packet: 0x%x\n", vertype);
- return;
+ if (m->m_pkthdr.len <= PPPOE_HEADERLEN) {
+ printf("pppoe: packet too short: %d\n", m->m_pkthdr.len);
+ goto done;
}
- code = *p++;
- PPPOE_READ_16(p, session);
- PPPOE_READ_16(p, plen);
- size -= PPPOE_HEADERLEN;
-
- if (plen > size) {
- printf("pppoe: packet content does not fit: data available = %ld, packet size = %ld\n",
- (long)size, (long)plen);
- return;
+
+ n = m_pulldown(m, off, sizeof(*ph), &noff);
+ if (!n) {
+ printf("pppoe: could not get PPPoE header\n");
+ goto done;
+ }
+ ph = (struct pppoehdr *)(mtod(n, caddr_t) + noff);
+ if (ph->vertype != PPPOE_VERTYPE) {
+ printf("pppoe: unknown version/type packet: 0x%x\n",
+ ph->vertype);
+ goto done;
+ }
+ session = ntohs(ph->session);
+ plen = ntohs(ph->plen);
+ off += sizeof(*ph);
+
+ if (plen + off > m->m_pkthdr.len) {
+ printf("pppoe: packet content does not fit: data available = %d, packet size = %u\n",
+ m->m_pkthdr.len - off, plen);
+ goto done;
}
- size = plen; /* ignore trailing garbage */
+ m_adj(m, off + plen - m->m_pkthdr.len); /* ignore trailing garbage */
tag = 0;
len = 0;
sc = NULL;
- while (size > 4) {
- PPPOE_READ_16(p, tag);
- PPPOE_READ_16(p, len);
- if (len > size) {
- printf("pppoe: tag 0x%x len 0x%x is too long\n", tag, len);
- return;
+ while (off + sizeof(*pt) < m->m_pkthdr.len) {
+ n = m_pulldown(m, off, sizeof(*pt), &noff);
+ if (!n)
+ break;
+ pt = (struct pppoetag *)(mtod(n, caddr_t) + noff);
+ tag = ntohs(pt->tag);
+ len = ntohs(pt->len);
+ if (off + len > m->m_pkthdr.len) {
+ printf("pppoe: tag 0x%x len 0x%x is too long\n",
+ tag, len);
+ goto done;
}
switch (tag) {
case PPPOE_TAG_EOL:
- size = 0; break;
+ goto breakbreak;
case PPPOE_TAG_SNAME:
break; /* ignored */
case PPPOE_TAG_ACNAME:
break; /* ignored */
case PPPOE_TAG_HUNIQUE:
- if (sc == NULL)
- sc = pppoe_find_softc_by_hunique(p, len, rcvif);
+ if (sc != NULL)
+ break;
+ n = m_pulldown(m, off + sizeof(*pt), len, &noff);
+ if (!n) {
+ err_msg = "TAG HUNIQUE ERROR";
+ break;
+ }
+ sc = pppoe_find_softc_by_hunique(mtod(n, caddr_t) + noff,
+ len, m->m_pkthdr.rcvif);
break;
case PPPOE_TAG_ACCOOKIE:
if (ac_cookie == NULL) {
- ac_cookie = p;
+ n = m_pulldown(m, off + sizeof(*pt), len,
+ &noff);
+ if (!n) {
+ err_msg = "TAG ACCOOKIE ERROR";
+ break;
+ }
+ ac_cookie = mtod(n, caddr_t) + noff;
ac_cookie_len = len;
}
break;
@@ -436,65 +477,69 @@
break;
}
if (err_msg) {
- printf("%s: %s\n", sc? sc->sc_sppp.pp_if.if_xname : "pppoe",
- err_msg);
- return;
- }
- if (size >= 0) {
- size -= 4 + len;
- if (len > 0)
- p += len;
+ printf("%s: %s\n",
+ sc ? sc->sc_sppp.pp_if.if_xname : "pppoe",
+ err_msg);
+ goto done;
}
+ off += sizeof(*pt) + len;
}
- switch (code) {
+breakbreak:;
+ switch (ph->code) {
case PPPOE_CODE_PADI:
case PPPOE_CODE_PADR:
/* ignore, we are no access concentrator */
- return;
+ goto done;
case PPPOE_CODE_PADO:
if (sc == NULL) {
/* be quiete if there is not a single pppoe instance */
if (!LIST_EMPTY(&pppoe_softc_list))
printf("pppoe: received PADO but could not find request for it\n");
- return;
+ goto done;
}
if (sc->sc_state != PPPOE_STATE_PADI_SENT) {
- printf("%s: received unexpected PADO\n", sc->sc_sppp.pp_if.if_xname);
- return;
+ printf("%s: received unexpected PADO\n",
+ sc->sc_sppp.pp_if.if_xname);
+ goto done;
}
if (ac_cookie) {
- sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF, M_DONTWAIT);
+ sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF,
+ M_DONTWAIT);
if (sc->sc_ac_cookie == NULL)
- return;
+ goto done;
sc->sc_ac_cookie_len = ac_cookie_len;
memcpy(sc->sc_ac_cookie, ac_cookie, ac_cookie_len);
}
- memcpy(&sc->sc_dest, eh->ether_shost, sizeof sc->sc_dest);
+ memcpy(&sc->sc_dest, eh.ether_shost, sizeof sc->sc_dest);
callout_stop(&sc->sc_timeout);
sc->sc_padr_retried = 0;
sc->sc_state = PPPOE_STATE_PADR_SENT;
if (pppoe_send_padr(sc) == 0)
- callout_reset(&sc->sc_timeout, PPPOE_DISC_TIMEOUT*(1+sc->sc_padr_retried), pppoe_timeout, sc);
+ callout_reset(&sc->sc_timeout,
+ PPPOE_DISC_TIMEOUT*(1+sc->sc_padr_retried),
+ pppoe_timeout, sc);
else
pppoe_abort_connect(sc);
break;
case PPPOE_CODE_PADS:
if (sc == NULL)
- return;
+ goto done;
sc->sc_session = session;
callout_stop(&sc->sc_timeout);
if (sc->sc_sppp.pp_if.if_flags & IFF_DEBUG)
- printf("%s: session 0x%x connected\n", sc->sc_sppp.pp_if.if_xname, session);
+ printf("%s: session 0x%x connected\n",
+ sc->sc_sppp.pp_if.if_xname, session);
sc->sc_state = PPPOE_STATE_SESSION;
sc->sc_sppp.pp_up(&sc->sc_sppp); /* notify upper layers */
break;
case PPPOE_CODE_PADT:
if (sc == NULL)
- return;
+ goto done;
/* stop timer (we might be about to transmit a PADT ourself) */
callout_stop(&sc->sc_timeout);
if (sc->sc_sppp.pp_if.if_flags & IFF_DEBUG)
- printf("%s: session 0x%x terminated, received PADT\n", sc->sc_sppp.pp_if.if_xname, session);
+ printf("%s: session 0x%x terminated, received PADT\n",
+ sc->sc_sppp.pp_if.if_xname, session);
/* clean up softc */
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(&sc->sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
@@ -510,61 +555,63 @@
default:
printf("%s: unknown code (0x%04x) session = 0x%04x\n",
sc? sc->sc_sppp.pp_if.if_xname : "pppoe",
- code, session);
+ ph->code, session);
break;
}
+
+done:
+ m_freem(m);
+ return;
}
static void
pppoe_disc_input(struct mbuf *m)
{
- u_int8_t *p;
- struct ether_header *eh;
/* avoid error messages if there is not a single pppoe instance */
if (!LIST_EMPTY(&pppoe_softc_list)) {
- eh = mtod(m, struct ether_header*);
- m_adj(m, sizeof(struct ether_header));
- p = mtod(m, u_int8_t*);
KASSERT(m->m_flags & M_PKTHDR);
- pppoe_dispatch_disc_pkt(p, m->m_len, m->m_pkthdr.rcvif, eh);
+ pppoe_dispatch_disc_pkt(m, 0);
}
- m_free(m);
}
static void
pppoe_data_input(struct mbuf *m)
{
- u_int8_t *p, vertype;
- u_int16_t session, plen, code;
+ u_int16_t session, plen;
struct pppoe_softc *sc;
+ struct pppoehdr *ph;
KASSERT(m->m_flags & M_PKTHDR);
m_adj(m, sizeof(struct ether_header));
if (m->m_pkthdr.len <= PPPOE_HEADERLEN) {
- printf("pppoe (data): dropping too short packet: %ld bytes\n", (long)m->m_pkthdr.len);
+ printf("pppoe (data): dropping too short packet: %d bytes\n",
+ m->m_pkthdr.len);
goto drop;
}
- p = mtod(m, u_int8_t*);
+ m = m_pullup(m, sizeof(*ph));
+ if (!m) {
+ printf("pppoe: could not get PPPoE header\n");
+ return;
+ }
+ ph = mtod(m, struct pppoehdr *);
- vertype = *p++;
- if (vertype != PPPOE_VERTYPE) {
- printf("pppoe (data): unknown version/type packet: 0x%x\n", vertype);
+ if (ph->vertype != PPPOE_VERTYPE) {
+ printf("pppoe (data): unknown version/type packet: 0x%x\n",
+ ph->vertype);
goto drop;
}
-
- code = *p++;
- if (code != 0)
+ if (ph->code != 0)
goto drop;
- PPPOE_READ_16(p, session);
+ session = ntohs(ph->session);
sc = pppoe_find_softc_by_session(session, m->m_pkthdr.rcvif);
if (sc == NULL)
goto drop;
- PPPOE_READ_16(p, plen);
+ plen = ntohs(ph->plen);
#if NBPFILTER > 0
if(sc->sc_sppp.pp_if.if_bpf)
@@ -588,7 +635,7 @@
printf("\n");
}
#endif
-
+
if (m->m_pkthdr.len < plen)
goto drop;
@@ -601,7 +648,7 @@
return;
drop:
- m_free(m);
+ m_freem(m);
}
static int
@@ -772,11 +819,12 @@
}
/* allocate a buffer */
- m0 = pppoe_get_mbuf(len+PPPOE_HEADERLEN); /* header len + payload len */
- if (!m0) return ENOBUFS;
+ m0 = pppoe_get_mbuf(len + PPPOE_HEADERLEN); /* header len + payload len */
+ if (!m0)
+ return ENOBUFS;
/* fill in pkt */
- p = mtod(m0, u_int8_t*);
+ p = mtod(m0, u_int8_t *);
PPPOE_ADD_HEADER(p, PPPOE_CODE_PADI, 0, len);
PPPOE_ADD_16(p, PPPOE_TAG_SNAME);
if (sc->sc_service_name != NULL) {
@@ -835,14 +883,14 @@
x = splnet();
sc->sc_padi_retried++;
if (sc->sc_padi_retried >= PPPOE_DISC_MAXPADI) {
- if ((sc->sc_sppp.pp_if.if_flags & IFF_LINK1) == 0
- && sc->sc_sppp.pp_if.if_ibytes) {
- /* slow retry mode */
- retry_wait = PPPOE_SLOW_RETRY;
+ if ((sc->sc_sppp.pp_if.if_flags & IFF_LINK1) == 0 &&
+ sc->sc_sppp.pp_if.if_ibytes) {
+ /* slow retry mode */
+ retry_wait = PPPOE_SLOW_RETRY;
} else {
- pppoe_abort_connect(sc);
- splx(x);
- return;
+ pppoe_abort_connect(sc);
+ splx(x);
+ return;
}
}
if (pppoe_send_padi(sc) == 0)
@@ -966,8 +1014,9 @@
}
if (sc->sc_ac_cookie_len > 0)
len += 2+2+sc->sc_ac_cookie_len; /* AC cookie */
- m0 = pppoe_get_mbuf(len+PPPOE_HEADERLEN);
- if (!m0) return ENOBUFS;
+ m0 = pppoe_get_mbuf(len + PPPOE_HEADERLEN);
+ if (!m0)
+ return ENOBUFS;
p = mtod(m0, u_int8_t*);
PPPOE_ADD_HEADER(p, PPPOE_CODE_PADR, 0, len);
PPPOE_ADD_16(p, PPPOE_TAG_SNAME);
@@ -1012,7 +1061,8 @@
printf("%s: sending PADT\n", sc->sc_sppp.pp_if.if_xname);
#endif
m0 = pppoe_get_mbuf(PPPOE_HEADERLEN);
- if (!m0) return ENOBUFS;
+ if (!m0)
+ return ENOBUFS;
p = mtod(m0, u_int8_t*);
PPPOE_ADD_HEADER(p, PPPOE_CODE_PADT, sc->sc_session, 0);
return pppoe_output(sc, m0);