On Wed, Oct 05, 2016 at 10:58 +0900, YASUOKA Masahiko wrote: > On Tue, 4 Oct 2016 17:27:12 +0200 > Mike Belopuhov <m...@belopuhov.com> wrote: > > On Tue, Oct 04, 2016 at 01:07 +0200, Vincent Gross wrote: > >> On Sat, 24 Sep 2016 10:58:10 +0200 > >> Vincent Gross <vgr...@openbsd.org> wrote: > >> > >> > Hi, > >> > > >> [snip] > >> > > >> > Aside from the mbuf issue, is this Ok ? > >> > >> I will go back on the mbuff stuff later. > >> > >> Diff rebased, ok anyone ? > >> > >> Index: net/if_vxlan.c > >> =================================================================== > >> RCS file: /cvs/src/sys/net/if_vxlan.c,v > >> retrieving revision 1.48 > >> diff -u -p -r1.48 if_vxlan.c > >> --- net/if_vxlan.c 30 Sep 2016 10:22:05 -0000 1.48 > >> +++ net/if_vxlan.c 3 Oct 2016 23:12:42 -0000 > >> @@ -638,7 +749,9 @@ vxlan_lookup(struct mbuf *m, struct udph > >> if (m->m_pkthdr.len < skip + sizeof(struct ether_header)) > >> return (EINVAL); > >> > >> - m_adj(m, skip); > >> + m_adj(m, skip - ETHER_ALIGN); > >> + m = m_pullup(m, ETHER_HDR_LEN + ETHER_ALIGN); > >> + m_adj(m, ETHER_ALIGN); > >> ifp = &sc->sc_ac.ac_if; > >> > >> #if NBRIDGE > 0 > > > > I think this chunk is correct. First you ensure that m->m_data > > points to a contiguous and well-aligned ethernet header and then > > you trim the alignment so that mtod() points directly at it. > > Isn't it possible that m_pullup() may return non aligned pointer? >
Do you mean if m->m_data pointer can be not aligned properly after an m_pullup? Of course, if the header layout is such that m_pullup doesn't need to move data around and at the same time the header that we're going to m_adj[ust] to is not aligned properly, this is going to be a problem. I wrote a test program (attached) that illustrates the change in behavior when compiled w/o any options versus when compiled with -DTEST3 (cc -DTEST3 -o mbuf mbuf.c) Meaning that m_pullup might not be enough in a generic case.
#include <sys/types.h> #include <sys/mbuf.h> #include <errno.h> #include <err.h> #include <string.h> #include <stdlib.h> #include <stdio.h> #define panic(x...) errx(1, x) unsigned int min(unsigned int a, unsigned int b) { return (a < b ? a : b); } void m_extfree(struct mbuf *m) { free(m->m_ext.ext_buf); m->m_flags &= ~(M_EXT|M_EXTWR); } struct mbuf * m_free(struct mbuf *m) { struct mbuf *n; if (m == NULL) return (NULL); n = m->m_next; if (m->m_flags & M_EXT) m_extfree(m); free(m); return (n); } struct mbuf * m_freem(struct mbuf *m) { struct mbuf *n; if (m == NULL) return (NULL); n = m->m_nextpkt; do m = m_free(m); while (m != NULL); return (n); } struct mbuf * m_get(int nowait, int type) { struct mbuf *m; m = calloc(1, sizeof(*m)); if (m == NULL) return (NULL); m->m_type = type; m->m_next = NULL; m->m_nextpkt = NULL; m->m_data = m->m_dat; m->m_flags = 0; return (m); } struct mbuf * m_inithdr(struct mbuf *m) { /* keep in sync with m_gethdr */ m->m_next = NULL; m->m_nextpkt = NULL; m->m_data = m->m_pktdat; m->m_flags = M_PKTHDR; memset(&m->m_pkthdr, 0, sizeof(m->m_pkthdr)); return (m); } struct mbuf * m_gethdr(int nowait, int type) { struct mbuf *m; m = calloc(1, sizeof(*m)); if (m == NULL) return (NULL); m->m_type = type; return (m_inithdr(m)); } struct mbuf * m_clget(struct mbuf *m, int how, u_int pktlen) { struct mbuf *m0 = NULL; caddr_t buf; if (m == NULL) { m0 = m_gethdr(how, MT_DATA); if (m0 == NULL) return (NULL); m = m0; } buf = calloc(1, pktlen); if (buf == NULL) { if (m0) m_freem(m0); return (NULL); } MEXTADD(m, buf, pktlen, M_EXTWR, MEXTFREE_POOL, NULL); return (m); } struct mbuf * m_getptr(struct mbuf *m, int loc, int *off) { while (loc >= 0) { /* Normal end of search */ if (m->m_len > loc) { *off = loc; return (m); } else { loc -= m->m_len; if (m->m_next == NULL) { if (loc == 0) { /* Point at the end of valid data */ *off = m->m_len; return (m); } else { return (NULL); } } else { m = m->m_next; } } } return (NULL); } int m_leadingspace(struct mbuf *m) { if (M_READONLY(m)) return 0; return (m->m_flags & M_EXT ? m->m_data - m->m_ext.ext_buf : m->m_flags & M_PKTHDR ? m->m_data - m->m_pktdat : m->m_data - m->m_dat); } int m_trailingspace(struct mbuf *m) { if (M_READONLY(m)) return 0; return (m->m_flags & M_EXT ? m->m_ext.ext_buf + m->m_ext.ext_size - (m->m_data + m->m_len) : &m->m_dat[MLEN] - (m->m_data + m->m_len)); } void m_copydata(struct mbuf *m, int off, int len, caddr_t cp) { unsigned count; if (off < 0) panic("m_copydata: off %d < 0", off); if (len < 0) panic("m_copydata: len %d < 0", len); if ((m = m_getptr(m, off, &off)) == NULL) panic("m_copydata: short mbuf chain"); while (len > 0) { if (m == NULL) panic("m_copydata: null mbuf"); count = min(m->m_len - off, len); memmove(cp, mtod(m, caddr_t) + off, count); len -= count; cp += count; off = 0; m = m->m_next; } } int m_copyback(struct mbuf *m0, int off, int len, const void *_cp, int wait) { int mlen, totlen = 0; struct mbuf *m = m0, *n; caddr_t cp = (caddr_t)_cp; int error = 0; if (m0 == NULL) return (0); while (off > (mlen = m->m_len)) { off -= mlen; totlen += mlen; if (m->m_next == NULL) { if ((n = m_get(wait, m->m_type)) == NULL) { error = ENOBUFS; goto out; } if (off + len > MLEN) { MCLGETI(n, wait, NULL, off + len); if (!(n->m_flags & M_EXT)) { m_free(n); error = ENOBUFS; goto out; } } memset(mtod(n, caddr_t), 0, off); n->m_len = len + off; m->m_next = n; } m = m->m_next; } while (len > 0) { /* extend last packet to be filled fully */ if (m->m_next == NULL && (len > m->m_len - off)) m->m_len += min(len - (m->m_len - off), M_TRAILINGSPACE(m)); mlen = min(m->m_len - off, len); memmove(mtod(m, caddr_t) + off, cp, mlen); cp += mlen; len -= mlen; totlen += mlen + off; if (len == 0) break; off = 0; if (m->m_next == NULL) { if ((n = m_get(wait, m->m_type)) == NULL) { error = ENOBUFS; goto out; } if (len > MLEN) { MCLGETI(n, wait, NULL, len); if (!(n->m_flags & M_EXT)) { m_free(n); error = ENOBUFS; goto out; } } n->m_len = len; m->m_next = n; } m = m->m_next; } out: if (((m = m0)->m_flags & M_PKTHDR) && (m->m_pkthdr.len < totlen)) m->m_pkthdr.len = totlen; return (error); } void m_adj(struct mbuf *mp, int req_len) { int len = req_len; struct mbuf *m; int count; if ((m = mp) == NULL) return; if (len >= 0) { /* * Trim from head. */ while (m != NULL && len > 0) { if (m->m_len <= len) { len -= m->m_len; m->m_len = 0; m = m->m_next; } else { m->m_len -= len; m->m_data += len; len = 0; } } if (mp->m_flags & M_PKTHDR) mp->m_pkthdr.len -= (req_len - len); } else { /* * Trim from tail. Scan the mbuf chain, * calculating its length and finding the last mbuf. * If the adjustment only affects this mbuf, then just * adjust and return. Otherwise, rescan and truncate * after the remaining size. */ len = -len; count = 0; for (;;) { count += m->m_len; if (m->m_next == NULL) break; m = m->m_next; } if (m->m_len >= len) { m->m_len -= len; if (mp->m_flags & M_PKTHDR) mp->m_pkthdr.len -= len; return; } count -= len; if (count < 0) count = 0; /* * Correct length for chain is "count". * Find the mbuf with last data, adjust its length, * and toss data from remaining mbufs on chain. */ m = mp; if (m->m_flags & M_PKTHDR) m->m_pkthdr.len = count; for (; m; m = m->m_next) { if (m->m_len >= count) { m->m_len = count; break; } count -= m->m_len; } while ((m = m->m_next) != NULL) m->m_len = 0; } } struct mbuf * m_pullup(struct mbuf *n, int len) { struct mbuf *m; int count; /* * If first mbuf has no cluster, and has room for len bytes * without shifting current data, pullup into it, * otherwise allocate a new mbuf to prepend to the chain. */ if ((n->m_flags & M_EXT) == 0 && n->m_next && n->m_data + len < &n->m_dat[MLEN]) { if (n->m_len >= len) return (n); m = n; n = n->m_next; len -= m->m_len; } else if ((n->m_flags & M_EXT) != 0 && len > MHLEN && n->m_next && n->m_data + len < &n->m_ext.ext_buf[n->m_ext.ext_size]) { if (n->m_len >= len) return (n); m = n; n = n->m_next; len -= m->m_len; } else { if (len > MAXMCLBYTES) goto bad; MGET(m, M_DONTWAIT, n->m_type); if (m == NULL) goto bad; if (len > MHLEN) { MCLGETI(m, M_DONTWAIT, NULL, len); if ((m->m_flags & M_EXT) == 0) { m_free(m); goto bad; } } m->m_len = 0; if (n->m_flags & M_PKTHDR) M_MOVE_PKTHDR(m, n); } do { count = min(len, n->m_len); memcpy(mtod(m, caddr_t) + m->m_len, mtod(n, caddr_t), count); len -= count; m->m_len += count; n->m_len -= count; if (n->m_len) n->m_data += count; else n = m_free(n); } while (len > 0 && n); if (len > 0) { (void)m_free(m); goto bad; } m->m_next = n; return (m); bad: m_freem(n); return (NULL); } void m_hexdump(struct mbuf *m) { char *cp, *desc, mark; int i, len, leading, trailing; while (m != NULL) { mark = ' '; len = MLEN; cp = m->m_dat; desc = "MBUF"; if (m->m_flags & M_PKTHDR) { len = MHLEN; cp = m->m_pktdat; desc = "PKTHDR"; } else if (m->m_flags & M_EXT) { len = m->m_ext.ext_size; cp = m->m_ext.ext_buf; desc = "CLUSTER"; } leading = m_leadingspace(m); trailing = m_trailingspace(m); printf("=== %s: len %d, total %d, leading(-) %d, trailing(+) %d\n", desc, m->m_len, len, leading, trailing); for (i = 0; i < len; i++) { if (i % 16 == 0) printf("%02x:", i); if (leading > 0 && i < leading ) mark = '-'; else if (trailing > 0 && i >= len - trailing) mark = '+'; printf(" %c%02x", mark, (unsigned char)*(cp + i)); if (i != 0 && (i % 16) == 15) printf("\n"); mark = ' '; } printf("\n"); m = m->m_next; } } int main(void) { struct mbuf *m_head, *m1; struct { uint32_t h1_1; uint32_t h1_2; #ifdef TEST3 uint16_t h1_3; #endif } __packed hdr1; struct { #if 1 uint16_t h2_pad[0]; /* no padding */ #else uint16_t h2_pad[1]; /* simulate ETHER_ALIGN */ #endif uint8_t h2_1; uint8_t h2_2; } __packed hdr2; uint8_t data[64]; MGETHDR(m_head, M_DONTWAIT, MT_DATA); if (m_head == NULL) err(1, "MGETHDR"); MGET(m1, M_DONTWAIT, MT_DATA); if (m1 == NULL) err(1, "MGET"); m_head->m_next = m1; memset(&hdr1, 0, sizeof(hdr1)); hdr1.h1_1 = htonl(0x11); hdr1.h1_2 = htonl(0x12); #ifdef TEST3 hdr1.h1_3 = htons(0x13); #endif memset(&hdr2, 0, sizeof(hdr2)); hdr2.h2_1 = 0x21; hdr2.h2_2 = 0x22; memset(data, 0xee, sizeof(data)); #if 1 /* Simulate empty first mbuf after an m_adj */ /* Put HDR1 into the first mbuf */ m_head->m_len = sizeof(hdr1); if (m_copyback(m_head, 0, sizeof(hdr1), &hdr1, M_DONTWAIT)) err(1, "m_copyback hdr1"); /* Put HDR2 into the second mbuf */ m1->m_len = sizeof(hdr2); if (m_copyback(m_head, sizeof(hdr1), sizeof(hdr2), &hdr2, M_DONTWAIT)) err(1, "m_copyback hdr2"); /* Put data into the second mbuf after an HDR2 */ m1->m_len += sizeof(data); if (m_copyback(m_head, sizeof(hdr1) + sizeof(hdr2), sizeof(data), data, M_DONTWAIT)) err(1, "m_copyback data"); #else /* Simulate a proper mbuf chain even after an m_adj */ /* Put HDR1 and HDR2 into the first mbuf */ m_head->m_len = sizeof(hdr1) + sizeof(hdr2); if (m_copyback(m_head, 0, sizeof(hdr1), &hdr1, M_DONTWAIT)) err(1, "m_copyback hdr1"); if (m_copyback(m_head, sizeof(hdr1), sizeof(hdr2), &hdr2, M_DONTWAIT)) err(1, "m_copyback hdr2"); /* Put data into the second mbuf after an HDR2 */ m1->m_len = sizeof(data); if (m_copyback(m_head, sizeof(hdr1) + sizeof(hdr2), sizeof(data), data, M_DONTWAIT)) err(1, "m_copyback data"); #endif m_head->m_pkthdr.len = m_head->m_len + m1->m_len; printf("\n*** SETUP ***\n\n"); m_hexdump(m_head); #if 0 /* This leaves first mbuf empty */ m_adj(m_head, sizeof(hdr1)); #else m_adj(m_head, sizeof(hdr1) - 2); m_head = m_pullup(m_head, sizeof(hdr2) + 2); m_adj(m_head, 2); #endif printf("\n*** RESULT ***\n\n"); m_hexdump(m_head); return (0); }