kern/168190: [pf] panic when using pf and route-to (maybe: bad fragment handling?)

[Daniel Hartmeier]

The following reply was made to PR kern/168190; it has been noted by GNATS.

From: Daniel Hartmeier <daniel at benzedrine.cx>
To: Joerg Pulz <Joerg.Pulz at frm2.tum.de>
Cc: bug-followup at freebsd.org, freebsd-pf at freebsd.org
Subject: Re: kern/168190: [pf] panic when using pf and route-to (maybe: bad fragment handling?)
Date: Mon, 4 Jun 2012 08:53:44 +0200

 --8t9RHnE3ZwKMSgU+
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 On Fri, Jun 01, 2012 at 10:25:39AM +0200, Joerg Pulz wrote:
 
 > panic: pf_test: 1: m->m_pkthdr.len 176, m->m_len 0
 > pf_test() at pf_test+0x259
 > pf_check_out() at pf_check_out+0x71
 > pfil_run_hooks() at pfil_run_hooks+0x113
 > ip_output() at ip_output+0x6de
 > ip_forward() at ip_forward+0x19e
 > ip_input() at ip_input+0x680
 > swi_net() at swi_net+0x15a
 
 The interesting part is in pfil_rule_hooks:
 
 > #12 0xffffffff8074adcf in pfil_run_hooks (ph=) at /usr/src/sys/net/pfil.c:89
 > 89				rv = (*pfh->pfil_func)(pfh->pfil_arg, &m, 
 > ifp, dir,
 > (kgdb) p *pfh
 > $6 = {pfil_link = {tqe_next = 0x0, tqe_prev = 0xfffffe0005821b00},
 >   pfil_func = 0xffffffff8032cc0a <pf_check_out>, pfil_arg = 0x0}
 
 There is a check on entry, which didn't trigger, so the mbuf was fine
 when the function was called.
 
 We're in the second pass of the loop, there seem to be (at least) two
 registered hooks, with pf being called second. What is the first one?
 You disabled ipfw, so my guess is ipfilter is first.
 
 Can you try to print *tqe_prev in the pfil_run_hooks frame?
 
 Now, the question is whether the first hook modifies the mbuf, or if
 it's pf on the way seen in your stack trace.
 
 I added further checks (before and after each hook), see the updated
 patch of pfil.c below.
 
 You could also disable ipfilter (so the module isn't loaded at all, and
 no pfil hook is registered).
 
 I'm reading more mbuf functions to find out what might leave the first
 chunk of an mbuf with m_len 0 (possibly some m_adj() call?), and from
 where it might be called.
 
 Kind regards,
 Daniel
 
 --8t9RHnE3ZwKMSgU+
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="pfil.diff"
 
 Index: sys/net/pfil.c
 ===================================================================
 RCS file: /home/ncvs/src/sys/net/pfil.c,v
 retrieving revision 1.19.2.1
 diff -u -r1.19.2.1 pfil.c
 --- sys/net/pfil.c	23 Sep 2011 00:51:37 -0000	1.19.2.1
 +++ sys/net/pfil.c	4 Jun 2012 06:39:46 -0000
 @@ -46,6 +46,8 @@
  
  #include <net/if.h>
  #include <net/pfil.h>
 +#include <netinet/in.h>
 +#include <netinet/ip.h>
  
  static struct mtx pfil_global_lock;
  
 @@ -74,15 +76,31 @@
  	struct mbuf *m = *mp;
  	int rv = 0;
  
 +	if (m->m_pkthdr.len < sizeof(struct ip) ||
 +	    m->m_len < sizeof(struct ip))
 +		panic("pfil_run_hooks: 1: m->m_pkthdr.len %d, m->m_len %d",
 +		    (int)m->m_pkthdr.len, (int)m->m_len);
  	PFIL_RLOCK(ph, &rmpt);
  	KASSERT(ph->ph_nhooks >= 0, ("Pfil hook count dropped < 0"));
  	for (pfh = pfil_hook_get(dir, ph); pfh != NULL;
  	     pfh = TAILQ_NEXT(pfh, pfil_link)) {
  		if (pfh->pfil_func != NULL) {
 +			ASSERT_HOST_BYTE_ORDER(m);
 +			if (m->m_pkthdr.len < sizeof(struct ip) ||
 +			    m->m_len < sizeof(struct ip))
 +				panic("pfil_run_hooks: 2: m->m_pkthdr.len %d, "
 +				    "m->m_len %d", (int)m->m_pkthdr.len,
 +				    (int)m->m_len);
  			rv = (*pfh->pfil_func)(pfh->pfil_arg, &m, ifp, dir,
  			    inp);
  			if (rv != 0 || m == NULL)
  				break;
 +			ASSERT_HOST_BYTE_ORDER(m);
 +			if (m->m_pkthdr.len < sizeof(struct ip) ||
 +			    m->m_len < sizeof(struct ip))
 +				panic("pfil_run_hooks: 3: m->m_pkthdr.len %d, "
 +				    "m->m_len %d", (int)m->m_pkthdr.len,
 +				    (int)m->m_len);
  		}
  	}
  	PFIL_RUNLOCK(ph, &rmpt);
 
 --8t9RHnE3ZwKMSgU+--