On Wed, Oct 05, 2016 at 20:36 +0200, Mike Belopuhov wrote: > On Wed, Oct 05, 2016 at 14:30 +0200, Mike Belopuhov wrote: > > 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. > > I have come up with a code that looks like this: > > struct mbuf *n; > int off; > > n = m_getptr(m_head, skip + ETHER_HDR_LEN, &off); > if ((m_head->m_len == m_head->m_pkthdr.len) && > ((n = m_getptr(m_head, skip + ETHER_HDR_LEN, &off)) == m_head) && > !ALIGNED_POINTER(mtod(n, char *) + off, uint32_t)) { > m_adj(m_head, skip - ETHER_ALIGN); > memmove(mtod(m_head, char *) - ETHER_ALIGN, > mtod(m_head, char *), m_head->m_len); > m_head->m_len -= ETHER_ALIGN; > } else if ((n = m_getptr(m_head, skip, &off)) != m_head) { > /* Move Ethernet header to the first mbuf */ > m_adj(m_head, skip - ETHER_HDR_LEN - ETHER_ALIGN); > m_copydata(n, off, ETHER_HDR_LEN, mtod(m_head, char *) + > ETHER_ALIGN); > m_adj(m_head, ETHER_ALIGN); > /* See if we need to shift the data */ > if (!ALIGNED_POINTER(mtod(n, char *) + ETHER_HDR_LEN, uint32_t)) { > memmove(mtod(n, char *), mtod(n, char *) + ETHER_HDR_LEN, > n->m_len - ETHER_HDR_LEN); > m_adj(n, -ETHER_HDR_LEN); > } else { > m_adj(n, ETHER_HDR_LEN); > } > } else { > m_adj(m_head, skip); > } > > But then I've realised that ether_input simply does m_adj(m, ETHER_HDR_LEN) > and passes the mbuf on to the ipv4_input where we do mtod(m, struct ip *). > Meaning that neither this funny code nor a simple m_pullup of the Ethernet > header will work. Ethernet header as well as an IP header must reside in > a contiguous space and be well aligned. Sigh.
Heh, rzalamena@ has noticed that ipv4_input does in fact do a pullup so that should prevent problems where IP header is in the different mbuf. However this doesn't solve the IP header alignment issue which is solved by my code above. So it's either that or a full copy of the packet via m_dup_pkt. I'm attaching an improved testing framework that I'm using.
#include <sys/types.h> #include <sys/mbuf.h> #include <assert.h> #include <errno.h> #include <err.h> #include <string.h> #include <stdlib.h> #include <stdio.h> /* sparc64 version */ #define XALIGNED_POINTER(p, t) ((((unsigned long)(p)) & (sizeof(t) - 1)) == 0) /* amd64 version */ #define ALIGNED_POINTER(p, t) 1 #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, 0, 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); } struct mbuf * m_prepend(struct mbuf *m, int len, int how) { struct mbuf *mn; if (len > MHLEN) panic("mbuf prepend length too big"); if (M_LEADINGSPACE(m) >= len) { m->m_data -= len; m->m_len += len; } else { MGET(mn, how, m->m_type); if (mn == NULL) { m_freem(m); return (NULL); } if (m->m_flags & M_PKTHDR) M_MOVE_PKTHDR(mn, m); mn->m_next = m; m = mn; MH_ALIGN(m, len); m->m_len = len; } if (m->m_flags & M_PKTHDR) m->m_pkthdr.len += len; return (m); } 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); } int m_dup_pkthdr(struct mbuf *to, struct mbuf *from, int wait) { assert(from->m_flags & M_PKTHDR); to->m_flags = (to->m_flags & (M_EXT | M_EXTWR)); to->m_flags |= (from->m_flags & M_COPYFLAGS); to->m_pkthdr = from->m_pkthdr; if ((to->m_flags & M_EXT) == 0) to->m_data = to->m_pktdat; return (0); } struct mbuf * m_dup_pkt(struct mbuf *m0, unsigned int adj, int wait) { struct mbuf *m; int len; len = m0->m_pkthdr.len + adj; if (len > MAXMCLBYTES) /* XXX */ return (NULL); m = m_get(wait, m0->m_type); if (m == NULL) return (NULL); if (m_dup_pkthdr(m, m0, wait) != 0) goto fail; if (len > MHLEN) { MCLGETI(m, wait, NULL, len); if (!(m->m_flags & M_EXT)) goto fail; } m->m_len = m->m_pkthdr.len = len; m_adj(m, adj); m_copydata(m0, 0, m0->m_pkthdr.len, mtod(m, caddr_t)); return (m); fail: m_freem(m); 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_EXT) { len = m->m_ext.ext_size; cp = m->m_ext.ext_buf; desc = "CLUSTER"; } else if (m->m_flags & M_PKTHDR) { len = MHLEN; cp = m->m_pktdat; desc = "PKTHDR"; } 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("%04x:", 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"); else if (i != 0 && (i % 4) == 3) printf(" |"); mark = ' '; } printf("\n"); m = m->m_next; } } #define ETHER_ALIGN 2 #define ETHER_ADDR_LEN 6 #define ETHER_TYPE_LEN 2 #define ETHER_HDR_LEN ((ETHER_ADDR_LEN * 2) + ETHER_TYPE_LEN) int main(void) { struct mbuf *m, *m_head = NULL; int offset, skip; char *solution = "unknown"; struct { u_int8_t ether_dhost[ETHER_ADDR_LEN]; u_int8_t ether_shost[ETHER_ADDR_LEN]; u_int16_t ether_type; } __packed eth_hdr; struct { u_int8_t ip_hl; u_int8_t ip_tos; u_int16_t ip_len; u_int16_t ip_id; u_int16_t ip_off; u_int8_t ip_ttl; u_int8_t ip_p; u_int16_t ip_sum; u_int32_t ip_src; u_int32_t ip_dst; } __packed ip_hdr; struct { u_int16_t uh_sport; u_int16_t uh_dport; u_int16_t uh_ulent; u_int16_t uh_sum; } __packed udp_hdr; struct { u_int32_t vxlan_flags; u_int32_t vxlan_id; } __packed vxlan_hdr; uint8_t data[64]; #if !defined(TEST3) && !defined(TEST4) m_head = MCLGETI(m_head, M_DONTWAIT, NULL, MCLBYTES); if (m_head == NULL) err(1, "MCLGET"); #ifndef TEST2 m_adj(m_head, ETHER_ALIGN); offset = ETHER_ALIGN; m_head->m_len += ETHER_ALIGN; #else offset = 0; #endif /* TEST2 */ #else /* TEST3 || TEST4 */ /* Simulate empty first mbuf after an m_adj */ struct mbuf *m1; MGETHDR(m_head, M_DONTWAIT, MT_DATA); if (m_head == NULL) err(1, "MGETHDR"); m_adj(m_head, ETHER_ALIGN); offset = ETHER_ALIGN; m_head->m_len += ETHER_ALIGN; MGET(m1, M_DONTWAIT, MT_DATA); if (m1 == NULL) err(1, "MGET"); m_head->m_next = m1; #endif m = m_head; /* Ethernet header */ memset(ð_hdr, 0x11, sizeof(eth_hdr)); m->m_len += sizeof(eth_hdr); if (m_copyback(m, offset, sizeof(eth_hdr), ð_hdr, M_DONTWAIT)) err(1, "m_copyback eth_hdr"); offset += sizeof(eth_hdr); /* IP header */ memset(&ip_hdr, 0x22, sizeof(ip_hdr)); m->m_len += sizeof(ip_hdr); if (m_copyback(m, offset, sizeof(ip_hdr), &ip_hdr, M_DONTWAIT)) err(1, "m_copyback ip_hdr"); offset += sizeof(ip_hdr); /* UDP header */ memset(&udp_hdr, 0x33, sizeof(udp_hdr)); m->m_len += sizeof(udp_hdr); if (m_copyback(m, offset, sizeof(udp_hdr), &udp_hdr, M_DONTWAIT)) err(1, "m_copyback udp_hdr"); offset += sizeof(udp_hdr); /* VXLAN header */ memset(&vxlan_hdr, 0x44, sizeof(vxlan_hdr)); m->m_len += sizeof(vxlan_hdr); if (m_copyback(m, offset, sizeof(vxlan_hdr), &vxlan_hdr, M_DONTWAIT)) err(1, "m_copyback vxlan_hdr"); offset += sizeof(vxlan_hdr); #if defined(TEST3) /* Move to the next mbuf */ offset = 0; m = m1; #elif defined(TEST4) /* Move to the next mbuf and add an offset */ offset = 2; m = m1; #endif /* Ethernet header */ memset(ð_hdr, 0x55, sizeof(eth_hdr)); m->m_len += sizeof(eth_hdr); if (m_copyback(m, offset, sizeof(eth_hdr), ð_hdr, M_DONTWAIT)) err(1, "m_copyback eth_hdr2"); offset += sizeof(eth_hdr); /* IP header */ memset(&ip_hdr, 0x66, sizeof(ip_hdr)); m->m_len += sizeof(ip_hdr); if (m_copyback(m, offset, sizeof(ip_hdr), &ip_hdr, M_DONTWAIT)) err(1, "m_copyback ip_hdr2"); offset += sizeof(ip_hdr); memset(data, 0xee, sizeof(data)); data[sizeof(data) - 1] = 0xff; m->m_len += sizeof(data); if (m_copyback(m, offset, sizeof(data), &data, M_DONTWAIT)) err(1, "m_copyback data"); m_head->m_pkthdr.len = m_head->m_len; #if defined(TEST3) || defined(TEST4) m_head->m_pkthdr.len += m1->m_len; #endif #if defined(TEST4) m_adj(m, 2); #endif printf("\n*** SETUP ***\n\n"); m_hexdump(m_head); skip = sizeof(eth_hdr) + sizeof(ip_hdr) + sizeof(udp_hdr) + sizeof(vxlan_hdr); #ifndef TEST2 skip += ETHER_ALIGN; #endif { struct mbuf *n; int off; n = m_getptr(m_head, skip + ETHER_HDR_LEN, &off); if ((m_head->m_len == m_head->m_pkthdr.len) && ((n = m_getptr(m_head, skip + ETHER_HDR_LEN, &off)) == m_head) && !XALIGNED_POINTER(mtod(n, char *) + off, uint32_t)) { m_adj(m_head, skip - ETHER_ALIGN); memmove(mtod(m_head, char *) - ETHER_ALIGN, mtod(m_head, char *), m_head->m_len); m_head->m_len -= ETHER_ALIGN; solution = "memmove"; } else if ((n = m_getptr(m_head, skip, &off)) != m_head) { /* Move Ethernet header to the first mbuf */ m_adj(m_head, skip - ETHER_HDR_LEN - ETHER_ALIGN); m_copydata(n, off, ETHER_HDR_LEN, mtod(m_head, char *) + ETHER_ALIGN); m_adj(m_head, ETHER_ALIGN); /* See if we need to shift the data */ if (!XALIGNED_POINTER(mtod(n, char *) + ETHER_HDR_LEN, uint32_t)) { memmove(mtod(n, char *), mtod(n, char *) + ETHER_HDR_LEN, n->m_len - ETHER_HDR_LEN); m_adj(n, -ETHER_HDR_LEN); solution = "m_copydata + memove"; } else { m_adj(n, ETHER_HDR_LEN); solution = "m_copydata + m_adj"; } } else { m_adj(m_head, skip); solution = "m_adj"; } } printf("\n*** RESULT (%s) ***\n\n", solution); m_hexdump(m_head); return (0); }