Date: Mon, 25 Apr 2005 15:46:46 -0700 From: Brooks Davis <brooks@one-eyed-alien.net> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: FreeBSD Current <freebsd-current@freebsd.org> Subject: Re: significant increase in ipfw pullup failed Message-ID: <20050425224646.GA6848@odin.ac.hmc.edu> In-Reply-To: <20050422214418.GB11806@odin.ac.hmc.edu> References: <17001.9557.627987.505930@roam.psg.com> <fdd333f2fb516561a8ded012141ac28d@mac.com> <17001.16520.774703.612151@roam.psg.com> <20050422112246.A70611@xorpc.icir.org> <17001.16764.512962.411616@roam.psg.com> <20050422115518.C70611@xorpc.icir.org> <20050422214418.GB11806@odin.ac.hmc.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
--T4sUOijqQbZv57TR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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...
> > >=20
> > > could be, no idea really. but fwiw, ipv6 is not enabled here.
> >=20
> > yes but there is some new code in the common path.
> > anyways i have cc-ed Brooks who committed the code
>=20
> 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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
@@ -328,11 +328,11 @@ 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))
=20
static __inline int
-icmptype_match(struct icmp *icmp, ipfw_insn_u32 *cmd)
+icmptype_match(struct icmphdr *icmp, ipfw_insn_u32 *cmd)
{
int type =3D icmp->icmp_type;
=20
@@ -343,7 +343,7 @@ icmptype_match(struct icmp *icmp, ipfw_i
(1 << ICMP_TSTAMP) | (1 << ICMP_IREQ) | (1 << ICMP_MASKREQ) )
=20
static int
-is_icmp_query(struct icmp *icmp)
+is_icmp_query(struct icmphdr *icmp)
{
int type =3D icmp->icmp_type;
=20
@@ -760,7 +760,7 @@ ipfw_log(struct ip_fw *f, u_int hlen, st
} else {
struct ip *ip =3D mtod(m, struct ip *);
/* these three are all aliases to the same thing */
- struct icmp *const icmp =3D L3HDR(struct icmp, ip);
+ struct icmphdr *const icmp =3D L3HDR(struct icmphdr, ip);
struct tcphdr *const tcp =3D (struct tcphdr *)icmp;
struct udphdr *const udp =3D (struct udphdr *)icmp;
=20
@@ -2108,11 +2108,7 @@ do { \
break;
=20
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 =3D ICMP(ulp)->icmp_type;
break;
=20
Index: netinet/ip_icmp.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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
@@ -49,6 +49,17 @@ 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 */
--=20
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
--T4sUOijqQbZv57TR
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)
iD8DBQFCbXNVXY6L6fI4GtQRAghcAJ9FzZfOTx/++DpFe+CX4vwKOhC+hgCfaUJQ
ddABVf5rR5j8IIIgFE05aGs=
=Tpxu
-----END PGP SIGNATURE-----
--T4sUOijqQbZv57TR--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050425224646.GA6848>