Re: significant increase in ipfw pullup failed

On Fri, Apr 22, 2005 at 02:44:18PM -0700, Brooks Davis wrote:
> On Fri, Apr 22, 2005 at 11:55:18AM -0700, Luigi Rizzo wrote:
> > On Fri, Apr 22, 2005 at 08:25:00AM -1000, Randy Bush wrote:
> > > > wonder if it is related to the recent import of ipfw v6 support...
> > > 
> > > could be, no idea really.  but fwiw, ipv6 is not enabled  here.
> > 
> > yes but there is some new code in the common path.
> > anyways i have cc-ed Brooks who committed the code
> 
> I suspect this is due to over agressive pullups of icmp packets (at
> least that's the most obvious place where the length changed) which are
> caused by poor design of the icmp struct.  We're pulling up the full
> length and should instead be pulling up 4 bytes.  I'm not sure what the
> best fix it.  Long term, creating a struct icmphdr is probably the right
> answer.  For now, the thing to do may be to add it and use it in ipfw,
> but not modify struct icmp just yet.

Here's a diff implementing that.  Could someone who is experinceing thse
extra warnings, please test it?

-- Brooks

Index: netinet/ip_fw2.c
===================================================================
RCS file: /usr/cvs/src/sys/netinet/ip_fw2.c,v
retrieving revision 1.95
diff -u -p -r1.95 ip_fw2.c
--- netinet/ip_fw2.c	19 Apr 2005 10:04:38 -0000	1.95
+++ netinet/ip_fw2.c	25 Apr 2005 21:57:30 -0000
_at__at_ -328,11 +328,11 _at__at_ SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, dy
 #define	L3HDR(T, ip)	((T *)((u_int32_t *)(ip) + (ip)->ip_hl))
 #define	TCP(p)		((struct tcphdr *)(p))
 #define	UDP(p)		((struct udphdr *)(p))
-#define	ICMP(p)		((struct icmp *)(p))
+#define	ICMP(p)		((struct icmphdr *)(p))
 #define	ICMP6(p)	((struct icmp6_hdr *)(p))
 
 static __inline int
-icmptype_match(struct icmp *icmp, ipfw_insn_u32 *cmd)
+icmptype_match(struct icmphdr *icmp, ipfw_insn_u32 *cmd)
 {
 	int type = icmp->icmp_type;
 
_at__at_ -343,7 +343,7 _at__at_ icmptype_match(struct icmp *icmp, ipfw_i
     (1 << ICMP_TSTAMP) | (1 << ICMP_IREQ) | (1 << ICMP_MASKREQ) )
 
 static int
-is_icmp_query(struct icmp *icmp)
+is_icmp_query(struct icmphdr *icmp)
 {
 	int type = icmp->icmp_type;
 
_at__at_ -760,7 +760,7 _at__at_ ipfw_log(struct ip_fw *f, u_int hlen, st
 	} else {
 		struct ip *ip = mtod(m, struct ip *);
 		/* these three are all aliases to the same thing */
-		struct icmp *const icmp = L3HDR(struct icmp, ip);
+		struct icmphdr *const icmp = L3HDR(struct icmphdr, ip);
 		struct tcphdr *const tcp = (struct tcphdr *)icmp;
 		struct udphdr *const udp = (struct udphdr *)icmp;
 
_at__at_ -2108,11 +2108,7 _at__at_ do {									\
 				break;
 
 			case IPPROTO_ICMP:
-				/*
-				 * we only care for 4 bytes: type, code,
-				 * checksum
-				 */
-				PULLUP_TO(hlen, ulp, struct icmp);
+				PULLUP_TO(hlen, ulp, struct icmphdr);
 				args->f_id.flags = ICMP(ulp)->icmp_type;
 				break;
 
Index: netinet/ip_icmp.h
===================================================================
RCS file: /usr/cvs/src/sys/netinet/ip_icmp.h,v
retrieving revision 1.24
diff -u -p -r1.24 ip_icmp.h
--- netinet/ip_icmp.h	21 Apr 2005 14:29:34 -0000	1.24
+++ netinet/ip_icmp.h	25 Apr 2005 22:44:43 -0000
_at__at_ -49,6 +49,17 _at__at_ struct icmp_ra_addr {
 /*
  * Structure of an icmp header.
  */
+struct icmphdr {
+	u_char	icmp_type;		/* type of message, see below */
+	u_char	icmp_code;		/* type sub code */
+	u_short	icmp_cksum;		/* ones complement cksum of struct */
+};
+
+/*
+ * Structure of an icmp packet.
+ *
+ * XXX: should start with a struct icmphdr.
+ */
 struct icmp {
 	u_char	icmp_type;		/* type of message, see below */
 	u_char	icmp_code;		/* type sub code */

-- 
Any statement of the form "X is the one, true Y" is FALSE.
PGP fingerprint 655D 519C 26A7 82E7 2529  9BF0 5D8E 8BE9 F238 1AD4