crashes in ipfilter on i386

[Greg Troxel]

I have an i386 running netbsd-4, and it's been crashing ever since I
upgraded recently. It is on an ethernet with v6 activity. I am getting
dumps for which I can't get useful backtraces, but dmesg -M shows the
fault address.

It's in frpr_icmp6, in fil.c, and it's the access to the 4th word of the
v6 src header (see the second line of IP6_NEQ call). (gcc puts the 4th
word first, then 3, 2, 1)

switch (icmp6->icmp6_type)
{
case ICMP6_ECHO_REPLY :
case ICMP6_ECHO_REQUEST :
minicmpsz = ICMP6ERR_MINPKTLEN - sizeof(ip6_t);
break;
case ICMP6_DST_UNREACH :
case ICMP6_PACKET_TOO_BIG :
case ICMP6_TIME_EXCEEDED :
case ICMP6_PARAM_PROB :
fin->fin_flx |= FI_ICMPERR;
if ((fin->fin_m != NULL) &&
(M_LEN(fin->fin_m) < fin->fin_plen)) {
if (fr_coalesce(fin) != 1)
return;
}

if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;

/*
* If the destination of this packet doesn't match the
* source of the original packet then this packet is
* not correct.
*/
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;

minicmpsz = ICMP6ERR_IPICMPHLEN - sizeof(ip6_t);
break;
default :
break;
}


ICMP6ERR_MINPKTLEN is 8+40
ICMPERR_ICMPHLEN is 8
ICMP6ERR_IPICMPHLEN is (40 + 8 + 40)

It seems at least odd to be using ICMPERR_ICMPHLEN with v6, but that
doesn't seem to be an issue.

Is the frpr_pullup ensuring that there is the outer IPv6 header, plus
the 8 bytes of icmp header, plus the whole source address from the
contained packet? I don't understand the rules about fin_dp - it seems
to be usd for alignment, and frpr_pullup seems to be be missing the
outer v6 header in the pullup amount.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Michael van Elst]

Post by Greg Troxel
I have an i386 running netbsd-4, and it's been crashing ever since I
upgraded recently.

Are you sure that you use this code?

Post by Greg Troxel
if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;

[...]

Post by Greg Troxel
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.

[Liam Foy]

Post by Michael van Elst

Post by Greg Troxel
I have an i386 running netbsd-4, and it's been crashing ever since I
upgraded recently.

Are you sure that you use this code?

Post by Greg Troxel
if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;

[...]

Post by Greg Troxel
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.

Which has also been pulled up to -4 if my memory is correct. Maybe try updating
your -4 tree and build a fresh kernel.

[Greg Troxel]

Post by Greg Troxel
I have an i386 running netbsd-4, and it's been crashing ever since I
upgraded recently.

Are you sure that you use this code?

I am pretty sure it was with the code with the frpr_pullup. If it
happens again I will nuke my OBJDIR and rebuild and reboot again. I
just installed a freshly built kernel (without cleaning out OBJDIR) this
morning and rebooted.



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

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: ***@serpens.de (Michael van Elst)
Subject: Re: crashes in ipfilter on i386
To: tech-***@netbsd.org
Date: Tue, 24 Jul 2007 20:00:04 +0000 (UTC)

Are you sure that you use this code?

Post by Greg Troxel
if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;

[...]

Post by Greg Troxel
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;
{
[<

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Michael van Elst
...
Are you sure that you use this code?

Post by Greg Troxel
if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;

[...]

Post by Greg Troxel
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?

I'd like you to try it without the #if 0 but with this change in ip_fil.h:

! #define I60(x) (((i6addr_t *)(x))->i6[0])
! #define I61(x) (((i6addr_t *)(x))->i6[1])
! #define I62(x) (((i6addr_t *)(x))->i6[2])
! #define I63(x) (((i6addr_t *)(x))->i6[3])
! #define HI60(x) ntohl(((i6addr_t *)(x))->i6[0])
! #define HI61(x) ntohl(((i6addr_t *)(x))->i6[1])
! #define HI62(x) ntohl(((i6addr_t *)(x))->i6[2])
! #define HI63(x) ntohl(((i6addr_t *)(x))->i6[3])

#define IP6_EQ(a,b) ((I63(a) == I63(b)) && (I62(a) ==
I62(b)) && \
(I61(a) == I61(b)) && (I60(a) == I60(b)))
--- 156,169 ----
#define iplookupptr vptr[0]
#define iplookupfunc lptr[1]

! #define I60(x) (((u_32_t *)(x))[0])
! #define I61(x) (((u_32_t *)(x))[1])
! #define I62(x) (((u_32_t *)(x))[2])
! #define I63(x) (((u_32_t *)(x))[3])
! #define HI60(x) ntohl(((u_32_t *)(x))[0])
! #define HI61(x) ntohl(((u_32_t *)(x))[1])
! #define HI62(x) ntohl(((u_32_t *)(x))[2])
! #define HI63(x) ntohl(((u_32_t *)(x))[3])

Darren


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

Post by Darren Reed

Post by Greg Troxel
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.

! #define I60(x) (((i6addr_t *)(x))->i6[0])
! #define I61(x) (((i6addr_t *)(x))->i6[1])
! #define I62(x) (((i6addr_t *)(x))->i6[2])
! #define I63(x) (((i6addr_t *)(x))->i6[3])
! #define HI60(x) ntohl(((i6addr_t *)(x))->i6[0])
! #define HI61(x) ntohl(((i6addr_t *)(x))->i6[1])
! #define HI62(x) ntohl(((i6addr_t *)(x))->i6[2])
! #define HI63(x) ntohl(((i6addr_t *)(x))->i6[3])
#define IP6_EQ(a,b) ((I63(a) == I63(b)) && (I62(a) ==
I62(b)) && \
(I61(a) == I61(b)) && (I60(a) == I60(b)))
--- 156,169 ----
#define iplookupptr vptr[0]
#define iplookupfunc lptr[1]
! #define I60(x) (((u_32_t *)(x))[0])
! #define I61(x) (((u_32_t *)(x))[1])
! #define I62(x) (((u_32_t *)(x))[2])
! #define I63(x) (((u_32_t *)(x))[3])
! #define HI60(x) ntohl(((u_32_t *)(x))[0])
! #define HI61(x) ntohl(((u_32_t *)(x))[1])
! #define HI62(x) ntohl(((u_32_t *)(x))[2])
! #define HI63(x) ntohl(((u_32_t *)(x))[3])

I might not have mentioned: the system I'm having trouble on is i386 and
netbsd-4. The above fix was for strict alignment machines and the
problem appeared on sparc64.

I already have that change; it's been pulled up to netbsd-4 - it's
precisely this commit:

revision 1.6.12.5
date: 2007/07/21 12:56:46; author: liamjfoy; state: Exp; lines: +10 -9
Pull up following revision(s) (requested by gdt in ticket #779):
sys/dist/ipf/netinet/fil.c: revision 1.39
sys/dist/ipf/netinet/ip_fil.h: revision 1.14
Avoid casting to "i6addr_t *", because that type requires 64-bit
alignment and nothing guarantees that IPv6 packets in mbufs are 8-byte
aligned. gcc was coalescing adjacent 32-bit compares into "ldx" on
sparc64, leading to alignment faults when processing icmp6 arriving on
gif with IPv4 outer addresses.
Fix mostly from ***@. Discussed extensively on port-sparc64.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Greg Troxel

Post by Darren Reed

Post by Greg Troxel
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.

! #define I60(x) (((i6addr_t *)(x))->i6[0])
! #define I61(x) (((i6addr_t *)(x))->i6[1])
! #define I62(x) (((i6addr_t *)(x))->i6[2])
! #define I63(x) (((i6addr_t *)(x))->i6[3])
! #define HI60(x) ntohl(((i6addr_t *)(x))->i6[0])
! #define HI61(x) ntohl(((i6addr_t *)(x))->i6[1])
! #define HI62(x) ntohl(((i6addr_t *)(x))->i6[2])
! #define HI63(x) ntohl(((i6addr_t *)(x))->i6[3])
#define IP6_EQ(a,b) ((I63(a) == I63(b)) && (I62(a) ==
I62(b)) && \
(I61(a) == I61(b)) && (I60(a) == I60(b)))
--- 156,169 ----
#define iplookupptr vptr[0]
#define iplookupfunc lptr[1]
! #define I60(x) (((u_32_t *)(x))[0])
! #define I61(x) (((u_32_t *)(x))[1])
! #define I62(x) (((u_32_t *)(x))[2])
! #define I63(x) (((u_32_t *)(x))[3])
! #define HI60(x) ntohl(((u_32_t *)(x))[0])
! #define HI61(x) ntohl(((u_32_t *)(x))[1])
! #define HI62(x) ntohl(((u_32_t *)(x))[2])
! #define HI63(x) ntohl(((u_32_t *)(x))[3])

I might not have mentioned: the system I'm having trouble on is i386 and
netbsd-4. The above fix was for strict alignment machines and the
problem appeared on sparc64.
I already have that change; it's been pulled up to netbsd-4 - it's
revision 1.6.12.5
date: 2007/07/21 12:56:46; author: liamjfoy; state: Exp; lines: +10 -9
sys/dist/ipf/netinet/fil.c: revision 1.39
sys/dist/ipf/netinet/ip_fil.h: revision 1.14
Avoid casting to "i6addr_t *", because that type requires 64-bit
alignment and nothing guarantees that IPv6 packets in mbufs are 8-byte
aligned. gcc was coalescing adjacent 32-bit compares into "ldx" on
sparc64, leading to alignment faults when processing icmp6 arriving on
gif with IPv4 outer addresses.

So you're saying that doesn't work?!
What assembly got generated as the result of the above?

Darren



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

So you're saying that doesn't work?!

That fixes the alignment crash on sparc64, and my sparc64 machine runs
fine. It is not stressed normally.

What assembly got generated as the result of the above?

It's really hard to follow (a maze of twisty inlines), but it looks ok.
My best guess is that under low-memory conditions there's a packet in an
mbuf at the edge of memory or a page and there is not enough data pulled
up. But I can't figure out what sizes are supposed to be there.
Perhaps we should add a KASSERT that the mbuf has enough before the
IP6_NEQ.

My i386 box has 2 GB of RAM, but it is often very busy (builds, 20G of
backups to tape).

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Darren Reed
So you're saying that doesn't work?!
That fixes the alignment crash on sparc64, and my sparc64 machine runs
fine. It is not stressed normally.
What assembly got generated as the result of the above?
It's really hard to follow (a maze of twisty inlines), but it looks ok.
My best guess is that under low-memory conditions there's a packet in an
mbuf at the edge of memory or a page and there is not enough data pulled
up. But I can't figure out what sizes are supposed to be there.
Perhaps we should add a KASSERT that the mbuf has enough before the
IP6_NEQ.
My i386 box has 2 GB of RAM, but it is often very busy (builds, 20G of
backups to tape).

Ok, that sound similar to a problem I ran into last night - an IPv6
packet where the ICMPV6 header seems to sit on the edge of a page.
I'm somewhat puzzled as to why the pullup doesn't fix this. And yes,
my experience was similar - after inlining, it is almost impossible
to work out what line of code is executing where. I had to -g it to
make sense of it (just removing the "inline" was not enough.)

Darren


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Greg Troxel]

So what I don't get is why

if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;

is adequate, since I think we need ICMP6ERR_IPICMPHLEN to get the
included header. But I do not understand how frpr_pullup and the data
pointer interact.


/*
* ICMP error replies have an IP header (20 bytes), 8 bytes of ICMP data,
* another IP header and then 64 bits of data, totalling 56. Of course,
* the last 64 bits is dependent on that being available.
*/
#define ICMPERR_ICMPHLEN 8
#define ICMPERR_IPICMPHLEN (20 + 8)
#define ICMPERR_MINPKTLEN (20 + 8 + 20)
#define ICMPERR_MAXPKTLEN (20 + 8 + 20 + 8)
#define ICMP6ERR_MINPKTLEN (40 + 8)
#define ICMP6ERR_IPICMPHLEN (40 + 8 + 40)


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Greg Troxel
So what I don't get is why
if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;
is adequate, since I think we need ICMP6ERR_IPICMPHLEN to get the
included header. But I do not understand how frpr_pullup and the data
pointer interact.

If we look in frpr_pullup(), we see:
static INLINE int frpr_pullup(fin, plen)
fr_info_t *fin;
int plen;
{
if (fin->fin_m != NULL) {
if (fin->fin_dp != NULL)
plen += (char *)fin->fin_dp -
((char *)fin->fin_ip + fin->fin_hlen);
plen += fin->fin_hlen;
if (M_LEN(fin->fin_m) < plen) {
if (fr_pullup(fin->fin_m, fin, plen) == NULL)
return -1;
}
}
return 0;
}

fin_dp is set early in fr_makefrip():
fin->fin_dp = (char *)ip + hlen;
...
} else if (v == 6) {
fin->fin_plen = ntohs(((ip6_t *)ip)->ip6_plen);
fin->fin_dlen = fin->fin_plen;
fin->fin_plen += hlen;

if (frpr_ipv6hdr(fin) == -1)
return -1;
...

In this case hlen will be 40 (sizeof(ip6_t)).
So if the payload length of the IPv6 header says 64 bytes,
fin_plen because 64+40=104.

In frpr_icmp6(), we do:

static INLINE void frpr_icmp6(fin)
fr_info_t *fin;
{
int minicmpsz = sizeof(struct icmp6_hdr);
struct icmp6_hdr *icmp6;

if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN - sizeof(ip6_t)) == -1)
return;

if (fin->fin_dlen > 1) {
ip6_t *ip6;

icmp6 = fin->fin_dp;
...
if ((fin->fin_m != NULL) &&
(M_LEN(fin->fin_m) < fin->fin_plen)) {
if (fr_coalesce(fin) != 1)
return;
}
*** if (frpr_pullup(fin, ICMP6ERR_MINPKTLEN) == -1)
return;

/*
* If the destination of this packet doesn't
match the
* source of the original packet then this packet is
* not correct.
*/
icmp6 = fin->fin_dp;
ip6 = (ip6_t *)((char *)icmp6 + ICMPERR_ICMPHLEN);
if (IP6_NEQ(&fin->fin_fi.fi_dst,
(i6addr_t *)&ip6->ip6_src))
fin->fin_flx |= FI_BAD;


The call to frpr_pullup() here is redundant - the call to fr_coalesce()
before it should have put everything in a single buffer.

int fr_coalesce(fin)
fr_info_t *fin;
{
if ((fin->fin_flx & FI_COALESCE) != 0)
return 1;

/*
* If the mbuf pointers indicate that there is no mbuf to work with,
* return but do not indicate success or failure.
*/
if (fin->fin_m == NULL || fin->fin_mp == NULL)
return 0;

if (fr_pullup(fin->fin_m, fin, fin->fin_plen) == NULL) {
ATOMIC_INCL(fr_badcoalesces[fin->fin_out]);
*fin->fin_mp = NULL;
fin->fin_m = NULL;
return -1;
}
return 1;
}

So a flag is set to prevent it from being called more than once.
Looking at fr_pullup(), there's nothing too surprising: fin_ip
and fin_dp are updated accordinly.

void *fr_pullup(xmin, fin, len)
mb_t *xmin;
fr_info_t *fin;
int len;
{
int out = fin->fin_out, dpoff, ipoff;
mb_t *m = xmin;
char *ip;

if (m == NULL)
return NULL;

ip = (char *)fin->fin_ip;
if ((fin->fin_flx & FI_COALESCE) != 0)
return ip;

ipoff = fin->fin_ipoff;
if (fin->fin_dp != NULL)
dpoff = (char *)fin->fin_dp - (char *)ip;
else
dpoff = 0;

if (M_LEN(m) < len) {
#ifdef MHLEN
/*
* Assume that M_PKTHDR is set and just work with what
is left
* rather than check..
* Should not make any real difference, anyway.
*/
if (len > MHLEN)
#else
if (len > MLEN)
#endif
{
#ifdef HAVE_M_PULLDOWN
if (m_pulldown(m, 0, len, NULL) == NULL)
m = NULL;
#else
FREE_MB_T(*fin->fin_mp);
m = NULL;
#endif
} else
{
m = m_pullup(m, len);
}
*fin->fin_mp = m;
fin->fin_m = m;
if (m == NULL) {
ATOMIC_INCL(frstats[out].fr_pull[1]);
return NULL;
}
ip = MTOD(m, char *) + ipoff;
}

ATOMIC_INCL(frstats[out].fr_pull[0]);
fin->fin_ip = (ip_t *)ip;
if (fin->fin_dp != NULL)
fin->fin_dp = (char *)fin->fin_ip + dpoff;

if (len == fin->fin_plen)
fin->fin_flx |= FI_COALESCE;
return ip;
}

Or is there something I'm missing?

Darren


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

So I've followed this through ...

fr_pullup() isn't doing the correct thing.

Some debugging printfs later...

mp 0xc2541700 m 0xc2541700 0x42 M_LEN 0 plen 243
ip 0xc25417d0 dp 0xc25417f8 hlen 40 dlen 203
MBUF 0xc2541700
data=0xc25417d0, len=0, type=1, flags=0x42<PKTHDR,LOOP>
owner=0x20202020, next=0xc2554800, nextpkt=0x0
leadingspace=152, trailingspace=48, readonly=0
pktlen=243, rcvif=0xc2a41c00, csum_flags=0x0, csum_data=0x280006, segsz=0
MBUF 0xc2554800
data=0xcc13f000, len=243, type=1, flags=0x9000001<EXT,EXT_CLUSTER,EXT_RW>
owner=0xffff, next=0xc2541c00, nextpkt=0x0
leadingspace=0, trailingspace=1805, readonly=0
shared=0, ext_buf=0xcc13f000, ext_size=2048, ext_free=0x0,
ext_arg=0xc04c6ec0
MBUF 0xc2541c00
data=0xc2541c3c, len=0, type=1, flags=0x40<LOOP>
owner=0xdeadbeef, next=0x0, nextpkt=0x0
leadingspace=28, trailingspace=196, readonly=0


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Pavel Cahyna]

Post by Darren Reed
So I've followed this through ...
fr_pullup() isn't doing the correct thing.

Try this (untested) patch and see if a message is printed before the
panic.

Index: ip_fil_netbsd.c
===================================================================
RCS file: /cvsroot/src/sys/dist/ipf/netinet/ip_fil_netbsd.c,v
retrieving revision 1.28.2.4
diff -u -p -r1.28.2.4 ip_fil_netbsd.c
--- ip_fil_netbsd.c 16 Jul 2007 11:05:41 -0000 1.28.2.4
+++ ip_fil_netbsd.c 16 Sep 2007 07:48:29 -0000
@@ -1906,6 +1906,9 @@ int len;
}
ip = MTOD(m, char *) + ipoff;
}
+ if (M_LEN(m) < len)
+ printf("fr_pullup malfunction,\n\
+size %d > %d, expect panic soon\n", (int)len, (int)M_LEN(m));

ATOMIC_INCL(frstats[out].fr_pull[0]);
fin->fin_ip = (ip_t *)ip;

Pavel

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de