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.