I am running netbsd-4 on i386, with every month of so rebuild/updates.
At some point around June (hazy), I started having occasional crashes,
and it pointed to fil.c in icmp6 code.  This was near where a sparc was
having alignment crashes.

I have now removed the IP6_NEQ statement, and my machine stayed up for
31 days, far longer than it recently had.  So I believe there is
something not right here, but I really don't know what.



  From: mlelstv@serpens.de (Michael van Elst)
  Subject:  Re: crashes in ipfilter on i386
  To: tech-net@netbsd.org
  Date: Tue, 24 Jul 2007 20:00:04 +0000 (UTC)

  Are you sure that you use this code?

  >                       if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
  >                               return;
  [...]
  >			icmp6 = fin->fin_dp;
  >			ip6 = (ip6_t *)((char *)icmp6 + ICMPERR_ICMPHLEN);
  >			if (IP6_NEQ(&fin->fin_fi.fi_dst,
  >				    &ip6->ip6_src))
  >				fin->fin_flx |= FI_BAD;

  I am asking, because there was a bug exactly in this place
  (a stale version of the icmp6 pointer was used) and the crash
  was exactly where you have shown it in your previous mail.

  However, this is fixed in the code snippet above.

  The frpr_pullup ensures that enough data is in the buffer for the
  IP6_NEQ operation and the icmp6 (and thus ip6) pointers are recomputed
  in case the buffer was moved.

I am sure that I was running the code with the frpr_pullup.


Here's the diff in my source tree, producing a kernel that doesn't
crash.  Note there is a lot of s/INLINE// noise because I found it hard
to follow the asm code, but the real change is dropping the IP6_NEQ
test.  Obviously my change causes a functionality regression - it was
intended to isolate the bug, not proposed as a fix.

I still suspect failure to pull up enough bytes, but I can't point to
the wrong line of code.

Any clues?


Index: sys/dist/ipf/netinet/fil.c
===================================================================
RCS file: /cvsroot/src/sys/dist/ipf/netinet/fil.c,v
retrieving revision 1.28.2.7
diff -u -p -r1.28.2.7 fil.c
--- sys/dist/ipf/netinet/fil.c	21 Jul 2007 12:56:45 -0000	1.28.2.7
+++ sys/dist/ipf/netinet/fil.c	10 Sep 2007 21:58:58 -0000
@@ -242,9 +242,9 @@ static	INLINE void	frpr_esp __P((fr_info
 static	INLINE void	frpr_gre __P((fr_info_t *));
 static	INLINE void	frpr_udp __P((fr_info_t *));
 static	INLINE void	frpr_tcp __P((fr_info_t *));
-static	INLINE void	frpr_icmp __P((fr_info_t *));
+static	 void	frpr_icmp __P((fr_info_t *));
 static	INLINE void	frpr_ipv4hdr __P((fr_info_t *));
-static	INLINE int	frpr_pullup __P((fr_info_t *, int));
+static int	frpr_pullup __P((fr_info_t *, int));
 static	INLINE void	frpr_short __P((fr_info_t *, int));
 static	INLINE int	frpr_tcpcommon __P((fr_info_t *));
 static	INLINE int	frpr_udpcommon __P((fr_info_t *));
@@ -357,7 +357,7 @@ static	INLINE void	frpr_esp6 __P((fr_inf
 static	INLINE void	frpr_gre6 __P((fr_info_t *));
 static	INLINE void	frpr_udp6 __P((fr_info_t *));
 static	INLINE void	frpr_tcp6 __P((fr_info_t *));
-static	INLINE void	frpr_icmp6 __P((fr_info_t *));
+static void	frpr_icmp6 __P((fr_info_t *));
 static	INLINE int	frpr_ipv6hdr __P((fr_info_t *));
 static	INLINE void	frpr_short6 __P((fr_info_t *, int));
 static	INLINE int	frpr_hopopts6 __P((fr_info_t *));
@@ -727,7 +727,7 @@ fr_info_t *fin;
 /* This routine is mainly concerned with determining the minimum valid size */
 /* for an ICMPv6 packet.                                                    */
 /* ------------------------------------------------------------------------ */
-static INLINE void frpr_icmp6(fin)
+static void frpr_icmp6(fin)
 fr_info_t *fin;
 {
 	int minicmpsz = sizeof(struct icmp6_hdr);
@@ -770,9 +770,11 @@ fr_info_t *fin;
 			 */
 			icmp6 = fin->fin_dp;
 			ip6 = (ip6_t *)((char *)icmp6 + ICMPERR_ICMPHLEN);
+#if 0
 			if (IP6_NEQ(&fin->fin_fi.fi_dst,
 				    &ip6->ip6_src))
 				fin->fin_flx |= FI_BAD;
+#endif
 
 			minicmpsz = ICMP6ERR_IPICMPHLEN - sizeof(ip6_t);
 			break;
@@ -916,7 +918,7 @@ fr_info_t *fin;
 /* to fr_pullup to ensure there is the required amount of data,             */
 /* consecutively in the packet buffer.                                      */
 /* ------------------------------------------------------------------------ */
-static INLINE int frpr_pullup(fin, plen)
+static int frpr_pullup(fin, plen)
 fr_info_t *fin;
 int plen;
 {
[<