Bug 148307

Summary:	[arge] [patch] Incorrect alignment checks in sys/mips/atheros/if_arge.c
Product:	Base System	Reporter:	Adrian Chadd <adrian>
Component:	kern	Assignee:	Adrian Chadd <adrian>
Status:	Closed FIXED
Severity:	Affects Only Me
Priority:	Normal
Version:	9.0-CURRENT
Hardware:	Any
OS:	Any

Description Adrian Chadd freebsd_committer

2010-07-02 07:50:01 UTC

2 bytes would appear in between the ethernet header and the
ethernet payload in some circumstances. In this particular
circumstance, it's when bridging over GRE tunnels. Traffic
from an ethernet connected host worked, but traffic from
a wireless connected host didn't.

The if_arge() TX code would check the first fragment to see
whether it's aligned and if not, a call to m_defrag() would
align and pack the fragments.

The hardware requires all fragments to be 4 bytes aligned -
and that all fragments save the last be a size that's
a multiple of 4 bytes.

In the case I was seeing, an encapsulated packet would be
an mbuf chain consisting of two parts. The first mbuf would
have a 14 byte ethernet header (6 byte src, 6 byte dst, 2 byte
ethertype) and then the original payload in the second mbuf.
This didn't trigger the call to m_defrag() and thus the
packet was passed along, untouched.

"Normal" traffic almost exclusively was misaligned and called
m_defrag().

Fix: 

The fix would be to check each mbuf in the TX chain for size
and alignment then call m_defrag() if the constraints are
missed.

My local fix was just to always call m_defrag() for now.
It is inefficient but it resolves the issue.
How-To-Repeat: 
Implement a pair of units which are bridging over GRE/IP:

* create bridge0
* create gre0, mtu 1500, setup IPs as needed
* throw arge1, wlan0, etc into bridge0
* throw gre0 into bridge0
* send traffic from wlan0 to a host on the other side of the
  bridge - this fails

Comment 1 Adrian Chadd freebsd_committer

2010-07-02 10:41:18 UTC

Responsible Changed
From-To: freebsd-bugs->adrian

I've got the fix, I'll talk with gonzo@ and commit an official 
fix soon.

Comment 2 Adrian Chadd freebsd_committer

2010-07-08 04:17:58 UTC

--- if_arge.c   (revision 209765)
+++ if_arge.c   (working copy)
@@ -818,6 +818,28 @@
 }

 /*
+ * Return whether the mbuf chain is correctly aligned
+ * for the arge TX engine.
+ *
+ * The TX engine requires each fragment to be aligned to a
+ * 4 byte boundary and the size of each fragment except
+ * the last to be a multiple of 4 bytes.
+ */
+static int
+arge_mbuf_chain_is_tx_aligned(struct mbuf *m0)
+{
+       struct mbuf *m;
+
+        for (m = m0; m != NULL; m = m->m_next) {
+               if((mtod(m, intptr_t) & 3) != 0)
+                       return 0;
+               if ((m->m_next != NULL) && ((m->m_len & 0x03) != 0))
+                       return 0;
+       }
+       return 1;
+}
+
+/*
  * Encapsulate an mbuf chain in a descriptor by coupling the mbuf data
  * pointers to the fragment pointers.
  */
@@ -837,7 +859,7 @@
         * even 4 bytes
         */
        m = *m_head;
-       if((mtod(m, intptr_t) & 3) != 0) {
+       if (! arge_mbuf_chain_is_tx_aligned(m)) {
                m = m_defrag(*m_head, M_DONTWAIT);
                if (m == NULL) {
                        *m_head = NULL;

Comment 3 dfilter service freebsd_committer

2010-07-08 15:59:46 UTC

Author: adrian
Date: Thu Jul  8 14:59:32 2010
New Revision: 209807
URL: http://svn.freebsd.org/changeset/base/209807

Log:
  Address PR kern/148307 - fix if_ath TX mbuf alignment/size constraint checks
  
  The existing code only checked the alignment of the first mbuf and
  didn't enforce the size constraints.
  
  This commit introduces a simple function to check the alignment and
  size of all mbufs in the list. This fixes the initial issue in the
  PR.
  
  PR: kern/148307
  Reviewed by: gonzo@

Modified:
  head/sys/mips/atheros/if_arge.c

Modified: head/sys/mips/atheros/if_arge.c
==============================================================================
--- head/sys/mips/atheros/if_arge.c	Thu Jul  8 14:56:42 2010	(r209806)
+++ head/sys/mips/atheros/if_arge.c	Thu Jul  8 14:59:32 2010	(r209807)
@@ -834,6 +834,28 @@ arge_init_locked(struct arge_softc *sc)
 }
 
 /*
+ * Return whether the mbuf chain is correctly aligned
+ * for the arge TX engine.
+ *
+ * The TX engine requires each fragment to be aligned to a
+ * 4 byte boundary and the size of each fragment except
+ * the last to be a multiple of 4 bytes.
+ */
+static int
+arge_mbuf_chain_is_tx_aligned(struct mbuf *m0)
+{
+	struct mbuf *m;
+
+	for (m = m0; m != NULL; m = m->m_next) {
+		if((mtod(m, intptr_t) & 3) != 0)
+			return 0;
+		if ((m->m_next != NULL) && ((m->m_len & 0x03) != 0))
+			return 0;
+	}
+	return 1;
+}
+
+/*
  * Encapsulate an mbuf chain in a descriptor by coupling the mbuf data
  * pointers to the fragment pointers.
  */
@@ -853,7 +875,7 @@ arge_encap(struct arge_softc *sc, struct
 	 * even 4 bytes
 	 */
 	m = *m_head;
-	if((mtod(m, intptr_t) & 3) != 0) {
+	if (! arge_mbuf_chain_is_tx_aligned(m)) {
 		m = m_defrag(*m_head, M_DONTWAIT);
 		if (m == NULL) {
 			*m_head = NULL;
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 4 Adrian Chadd freebsd_committer

2011-04-09 04:10:56 UTC

State Changed
From-To: open->closed

Committed to -HEAD