Discussion:
alignment fault on armv7 when using carp(4)
David Gwynne
2016-02-09 10:31:14 UTC
Permalink
Synopsis: <alignment fault on armv7 (omap) using carp(4)>
To me that behavior might suggest the problem is deeper than a
bookkeeping mistake of aligning memory in mbuf.
nope, you were right, it's a screwup with alignment.
the problem is multicast packets that arent to a carp interfaces
mac address have to be duplicated and sent to all carp interfaces
on a parent. the duplication is done with m_copym2, which doesn't
respect the alignment requirements of the ip header inside the 14
byte ethernet header.
the following dups the packet inside carp, and makes sure the
ethernet payload is aligned properly.
i was able to reproduce this on sparc64, and i believe this fixes
it. could you test it and see if it helps?
mpi@ pointed out that bridge@ has a special function to do a deep
copy of mbufs that get the ip payload alignment right, and that we
should share.

this moves the functionality in with the rest of the mbuf functions.

could a bridge user test this to see if it still works? carp seems
fine with this on sparc64 stil.

ok?

Index: kern/uipc_mbuf.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.218
diff -u -p -r1.218 uipc_mbuf.c
--- kern/uipc_mbuf.c 31 Jan 2016 00:18:07 -0000 1.218
+++ kern/uipc_mbuf.c 9 Feb 2016 09:49:21 -0000
@@ -1213,6 +1213,40 @@ m_dup_pkthdr(struct mbuf *to, struct mbu
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(m0->m_type, wait);
+ if (m == NULL)
+ return (NULL);
+
+ if (m_dup_pkthdr(m, m0, wait) != 0)
+ goto fail;
+
+ if (len > MHLEN) {
+ MCLGETI(m, len, NULL, wait);
+ if (!ISSET(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);
+}
+
#ifdef DDB
void
m_print(void *v,
Index: net/if_bridge.c
===================================================================
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.275
diff -u -p -r1.275 if_bridge.c
--- net/if_bridge.c 5 Dec 2015 10:07:55 -0000 1.275
+++ net/if_bridge.c 9 Feb 2016 09:49:21 -0000
@@ -137,7 +137,6 @@ int bridge_ipsec(struct bridge_softc *,
int bridge_clone_create(struct if_clone *, int);
int bridge_clone_destroy(struct ifnet *ifp);
int bridge_delete(struct bridge_softc *, struct bridge_iflist *);
-struct mbuf *bridge_m_dup(struct mbuf *);

#define ETHERADDR_IS_IP_MCAST(a) \
/* struct etheraddr *a; */ \
@@ -800,7 +799,7 @@ bridge_output(struct ifnet *ifp, struct
used = 1;
mc = m;
} else {
- mc = bridge_m_dup(m);
+ mc = m_dup_pkt(m, ETHER_ALIGN, M_DONTWAIT);
if (mc == NULL) {
sc->sc_if.if_oerrors++;
continue;
@@ -1090,7 +1089,7 @@ bridge_process(struct ifnet *ifp, struct
(ifl->bif_state == BSTP_IFSTATE_DISCARDING))
goto reenqueue;

- mc = bridge_m_dup(m);
+ mc = m_dup_pkt(m, ETHER_ALIGN, M_DONTWAIT);
if (mc == NULL)
goto reenqueue;

@@ -1227,7 +1226,7 @@ bridge_broadcast(struct bridge_softc *sc
mc = m;
used = 1;
} else {
- mc = bridge_m_dup(m);
+ mc = m_dup_pkt(m, ETHER_ALIGN, M_DONTWAIT);
if (mc == NULL) {
sc->sc_if.if_oerrors++;
continue;
@@ -1277,7 +1276,7 @@ bridge_localbroadcast(struct bridge_soft
return;
}

- m1 = bridge_m_dup(m);
+ m1 = m_dup_pkt(m, ETHER_ALIGN, M_DONTWAIT);
if (m1 == NULL) {
sc->sc_if.if_oerrors++;
return;
@@ -2017,37 +2016,4 @@ bridge_copyaddr(struct sockaddr *src, st
memcpy(dst, src, src->sa_len);
else
dst->sa_family = AF_UNSPEC;
-}
-
-/*
- * Specialized deep copy to ensure that the payload after the Ethernet
- * header is nicely aligned.
- */
-struct mbuf *
-bridge_m_dup(struct mbuf *m)
-{
- struct mbuf *m1, *m2, *mx;
-
- m1 = m_copym2(m, 0, ETHER_HDR_LEN, M_DONTWAIT);
- if (m1 == NULL) {
- return (NULL);
- }
- m2 = m_copym2(m, ETHER_HDR_LEN, M_COPYALL, M_DONTWAIT);
- if (m2 == NULL) {
- m_freem(m1);
- return (NULL);
- }
-
- for (mx = m1; mx->m_next != NULL; mx = mx->m_next)
- /*EMPTY*/;
- mx->m_next = m2;
-
- if (m1->m_flags & M_PKTHDR) {
- int len = 0;
- for (mx = m1; mx != NULL; mx = mx->m_next)
- len += mx->m_len;
- m1->m_pkthdr.len = len;
- }
-
- return (m1);
}
Index: netinet/ip_carp.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.286
diff -u -p -r1.286 ip_carp.c
--- netinet/ip_carp.c 21 Jan 2016 11:23:48 -0000 1.286
+++ netinet/ip_carp.c 9 Feb 2016 09:49:21 -0000
@@ -210,6 +210,7 @@ int carp_input(struct ifnet *, struct mb
void carp_proto_input_c(struct ifnet *, struct mbuf *,
struct carp_header *, int, sa_family_t);
void carp_proto_input_if(struct ifnet *, struct mbuf *, int);
+int carp_input_mcast(struct carp_softc *, struct mbuf *);
void carpattach(int);
void carpdetach(struct carp_softc *);
int carp_prepare_ad(struct mbuf *, struct carp_vhost_entry *,
@@ -1429,9 +1430,10 @@ carp_input(struct ifnet *ifp0, struct mb
if (!(sc->sc_if.if_flags & IFF_UP))
continue;

- m0 = m_copym2(m, 0, M_COPYALL, M_DONTWAIT);
+ m0 = m_dup_pkt(m, ETHER_ALIGN, M_DONTWAIT);
+ /* if we cant send one we probably cant send more */
if (m0 == NULL)
- continue;
+ break;

ml_init(&ml);
ml_enqueue(&ml, m0);
Index: sys/mbuf.h
===================================================================
RCS file: /cvs/src/sys/sys/mbuf.h,v
retrieving revision 1.207
diff -u -p -r1.207 mbuf.h
--- sys/mbuf.h 31 Jan 2016 00:18:07 -0000 1.207
+++ sys/mbuf.h 9 Feb 2016 09:49:21 -0000
@@ -447,6 +447,7 @@ void m_cat(struct mbuf *, struct mbuf *)
struct mbuf *m_devget(char *, int, int);
int m_apply(struct mbuf *, int, int,
int (*)(caddr_t, caddr_t, unsigned int), caddr_t);
+struct mbuf *m_dup_pkt(struct mbuf *, unsigned int, int);
int m_dup_pkthdr(struct mbuf *, struct mbuf *, int);

/* Packet tag routines */
Mike Belopuhov
2016-02-09 11:12:06 UTC
Permalink
Post by David Gwynne
Synopsis: <alignment fault on armv7 (omap) using carp(4)>
To me that behavior might suggest the problem is deeper than a
bookkeeping mistake of aligning memory in mbuf.
nope, you were right, it's a screwup with alignment.
the problem is multicast packets that arent to a carp interfaces
mac address have to be duplicated and sent to all carp interfaces
on a parent. the duplication is done with m_copym2, which doesn't
respect the alignment requirements of the ip header inside the 14
byte ethernet header.
the following dups the packet inside carp, and makes sure the
ethernet payload is aligned properly.
i was able to reproduce this on sparc64, and i believe this fixes
it. could you test it and see if it helps?
copy of mbufs that get the ip payload alignment right, and that we
should share.
this moves the functionality in with the rest of the mbuf functions.
could a bridge user test this to see if it still works? carp seems
fine with this on sparc64 stil.
ok?
m_adj can be done as part of the m_copym2 as well.
In the long run I don't think that introducing a new function
makes sense, not sure about 5.9 and right now, though.
David Gwynne
2016-02-09 11:19:22 UTC
Permalink
Post by Mike Belopuhov
Post by David Gwynne
Synopsis: <alignment fault on armv7 (omap) using carp(4)>
To me that behavior might suggest the problem is deeper than a
bookkeeping mistake of aligning memory in mbuf.
nope, you were right, it's a screwup with alignment.
the problem is multicast packets that arent to a carp interfaces
mac address have to be duplicated and sent to all carp interfaces
on a parent. the duplication is done with m_copym2, which doesn't
respect the alignment requirements of the ip header inside the 14
byte ethernet header.
the following dups the packet inside carp, and makes sure the
ethernet payload is aligned properly.
i was able to reproduce this on sparc64, and i believe this fixes
it. could you test it and see if it helps?
copy of mbufs that get the ip payload alignment right, and that we
should share.
this moves the functionality in with the rest of the mbuf functions.
could a bridge user test this to see if it still works? carp seems
fine with this on sparc64 stil.
ok?
m_adj can be done as part of the m_copym2 as well.
you want to shove m_adj into m_copym2? or you want m_copym2 callers to m_prepend 2 bytes first?
Post by Mike Belopuhov
In the long run I don't think that introducing a new function
makes sense, not sure about 5.9 and right now, though.
im not sure using m_copym2 for a deep copy makes that much sense generally. it's not a great implementation, and the vast majority of the callers use it to copy everything.
Mike Belopuhov
2016-02-09 11:37:58 UTC
Permalink
Post by David Gwynne
Post by Mike Belopuhov
Post by David Gwynne
Synopsis: <alignment fault on armv7 (omap) using carp(4)>
To me that behavior might suggest the problem is deeper than a
bookkeeping mistake of aligning memory in mbuf.
nope, you were right, it's a screwup with alignment.
the problem is multicast packets that arent to a carp interfaces
mac address have to be duplicated and sent to all carp interfaces
on a parent. the duplication is done with m_copym2, which doesn't
respect the alignment requirements of the ip header inside the 14
byte ethernet header.
the following dups the packet inside carp, and makes sure the
ethernet payload is aligned properly.
i was able to reproduce this on sparc64, and i believe this fixes
it. could you test it and see if it helps?
copy of mbufs that get the ip payload alignment right, and that we
should share.
this moves the functionality in with the rest of the mbuf functions.
could a bridge user test this to see if it still works? carp seems
fine with this on sparc64 stil.
ok?
m_adj can be done as part of the m_copym2 as well.
you want to shove m_adj into m_copym2? or you want m_copym2 callers to m_prepend 2 bytes first?
Post by Mike Belopuhov
In the long run I don't think that introducing a new function
makes sense, not sure about 5.9 and right now, though.
im not sure using m_copym2 for a deep copy makes that much sense generally. it's not a great implementation, and the vast majority of the callers use it to copy everything.
after talking to dlg@ i think that I'd prefer something like m_copypacket
from FreeBSD that preserves the alignment of the first mbuf in the chain.
Loading...