pf panic trace [Was: Re: Return-icmp doesn't work]

[Pyun YongHyeon]

On Fri, Mar 11, 2005 at 05:12:31PM +0100, Emanuel Strobl wrote:
 > Am Freitag, 11. M?rz 2005 16:19 schrieb Emanuel Strobl:
 > > Am Freitag, 11. M?rz 2005 14:52 schrieb Daniel Hartmeier:
 > > >   block return-rst in on wi0 reply-to (wi0 10.1.1.1) inet proto tcp all
 > > >
 > > > This is valid syntax and pfctl loads the rule, but the functionality is
 > > > not implemented in kernel yet, i.e. the reply-to option is simply
 > > > ignored.
 > >
 > > Thanks, I tried a very similar rule and after that the box vanished.
 > > I went on location (the box paniced but didn't reboot) and installed a
 > > console-server so I can access the box from here and currently I'm baking a
 > > debug kernel.
 > > I'll notify you if I have a trace!
 > 
 > Here's the original panic message (the non debug kernel) with 5.4-PRE one week 
 > old:
 > Fatal trap 12: page fault while in kernel mode
 > fault virtual address   = 0xc
 > fault code              = supervisor read, page not pre
 > instruction pointer     = 0x8:0xc05ac722
 > stack pointer           = 0x10:0xcc6919ac
 > frame pointer           = 0x10:0xcc6919e0
 > code segment            = base 0x0, limit 0xfffff, type
 >                         = DPL 0, pres 1, def32 1, gran
 > processor eflags        = interrupt enabled, resume, IO
 > current process         = 34 (swi1: net)
 > trap number             = 12
 > panic: page fault
 > Uptime: 1d1h20m33s
 > GEOM_MIRROR: Device web: provider mirror/web destroyed.
 > GEOM_MIRROR: Device web destroyed.
 > ...
 > The machine didn't reboot!
 > 
 > 
 > The following rule panickes the machine:
 > block return-icmp(13) in on $SDSL route-to ($SDSL $sdsl_gw) from any to 
 > $sdsl_net
 > 
 > Here's the trace from 5.4-PRE today:
 > panic: m_copym, offset > size of mbuf chain
 > KDB: stack backtrace:
 > panic(c076ab9a,c174d500,100,cc694a30,0) at panic+0x13c
 > m_copym(c1621b00,5dc,5c8,1,14) at m_copym+0x1c7
 > ip_fragment(c1642010,cc694a74,5dc,6,f01) at ip_fragment+0x168
 > pf_route(cc694bf0,c1a10d20,1,c1585000,0) at pf_route+0x767
 > pf_test(1,c1585000,cc694bf0,0,c17554e0) at pf_test+0x7b1
 > pf_check_in(0,cc694bf0,c1585000,1,0) at pf_check_in+0x48
 > pfil_run_hooks(c07f3e60,cc694c9c,c1585000,1,0) at pfil_run_hooks+0x15b
 > ip_input(c1621b00,0,c076e621,e6,c07f3f20) at ip_input+0x20f
 > netisr_processqueue(cc694cd8,246,c07c8ee0,2,c1508d40) at 
 > netisr_processqueue+0x15
 > swi_net(0,0,c0762ddc,269,0) at swi_net+0x8d
 > ithread_loop(c1526300,cc694d48,c0762bbd,30e,0) at ithread_loop+0x1ff
 > fork_exit(c0560640,c1526300,cc694d48) at fork_exit+0xa9
 > fork_trampoline() at fork_trampoline+0x8
 > --- trap 0x1, eip = 0, esp = 0xcc694d7c, ebp = 0 ---
 > 
 > If you need more info, on http://www.schmalzbauer.de/statics/phobos you can 
 > find dmesg and the whole pf.conf
 > 

Hmm, Max and I had seen these kind of traces when pf porting
was in progress. But now I believe we fixed all possible
cases.

I can't sure but your trace indicates there is a bug in
ip_fragment(). If a packet already set IP_MF flag in ip header,
we would get invalid ip_off in fragmented packet.
And it seems that there is another bug in pf. Since ip_fragment()
can change passed mbuf, we should not use saved copy of it.
Untested patch for CURRENT attached.

-- 
Regards,
Pyun YongHyeon
http://www.kr.freebsd.org/~yongari	|	yongari at freebsd.org
-------------- next part --------------
--- sys/netinet/ip_output.c.orig	Tue Mar  1 20:57:14 2005
+++ sys/netinet/ip_output.c	Sat Mar 12 13:50:54 2005
@@ -959,7 +959,7 @@
 		}
 		m->m_len = mhlen;
 		/* XXX do we need to add ip->ip_off below ? */
-		mhip->ip_off = ((off - hlen) >> 3) + ip->ip_off;
+		mhip->ip_off = ((off - hlen) >> 3) + (ip->ip_off & ~IP_MF);
 		if (off + len >= ip->ip_len) {	/* last fragment */
 			len = ip->ip_len - off;
 			m->m_flags |= M_LASTFRAG;
-------------- next part --------------
--- sys/contrib/pf/net/pf.c.orig	Tue Feb 22 10:30:53 2005
+++ sys/contrib/pf/net/pf.c	Sat Mar 12 12:42:37 2005
@@ -5421,7 +5421,6 @@
 			goto bad;
 	}
 
-	m1 = m0;
 #ifdef __FreeBSD__
 	/*
 	 * XXX: is cheaper + less error prone than own function
@@ -5430,6 +5429,7 @@
 	NTOHS(ip->ip_off);
 	error = ip_fragment(ip, &m0, ifp->if_mtu, ifp->if_hwassist, sw_csum);
 #else
+	m1 = m0;
 	error = ip_fragment(m0, ifp, ifp->if_mtu);
 #endif
 	if (error) {
@@ -5439,10 +5439,14 @@
 		goto bad;
 	}
 
+#ifdef __FreeBSD__
+	for (; m0; m0 = m1) {
+#else
 	for (m0 = m1; m0; m0 = m1) {
+#endif
 		m1 = m0->m_nextpkt;
-		m0->m_nextpkt = 0;
 #ifdef __FreeBSD__
+		m0->m_nextpkt = NULL;
 		if (error == 0) {
 			PF_UNLOCK();
 			error = (*ifp->if_output)(ifp, m0, sintosa(dst),
@@ -5450,6 +5454,7 @@
 			PF_LOCK();
 		} else
 #else
+		m0->m_nextpkt = 0;
 		if (error == 0)
 			error = (*ifp->if_output)(ifp, m0, sintosa(dst),
 			    NULL);