Subject: Re: crashes in ipfilter on i386
To: Darren Reed <darrenr@netbsd.org>
From: Greg Troxel <gdt@ir.bbn.com>
List: tech-net
Date: 09/13/2007 09:43:03
Darren Reed <darrenr@netbsd.org> writes:

> Greg Troxel wrote:
>
>> 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.
>
> 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])

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 darrenr@.  Discussed extensively on port-sparc64.