Greg,

The comment should go in ip_compat.h, not fil.c.
Otherwise, it looks good to go.

Darren

Greg Troxel wrote:
>   From: Darren Reed <darrenr@netbsd.org>
> 
>   I think the correct thing to do is change these:
> 
>   #define I60(x)  (((i6addr_t *)(x))->i6[0])
>   [rest trimmed]
> 
>   to be:
> 
>   #define I60(x)  (((u_32_t *)(x))[0])
> 
> I did that and the resulting kernel doesn't crash and traceroute6
> through it works.
> 
> Here's a patch against netbsd-4, which also includes a caution about a
> remaining unsafe cast.  Shall I apply it to current and request a pullup
> (I've tested on netbsd-4 sparc64 only, and not really tested that there
> are no functional problems), or would you like to handle this?
> 
> Index: sys/dist/ipf/netinet/fil.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dist/ipf/netinet/fil.c,v
> retrieving revision 1.28.2.6
> diff -u -p -r1.28.2.6 fil.c
> --- sys/dist/ipf/netinet/fil.c	16 Jul 2007 11:08:45 -0000	1.28.2.6
> +++ sys/dist/ipf/netinet/fil.c	17 Jul 2007 14:39:00 -0000
> @@ -770,6 +770,11 @@ fr_info_t *fin;
>  			 */
>  			icmp6 = fin->fin_dp;
>  			ip6 = (ip6_t *)((char *)icmp6 + ICMPERR_ICMPHLEN);
> +			/*
> +			 * XXX cast to i6addr_t is unsafe because it
> +			 * presumes void * alignment which may not be
> +			 * true, but IP6_NEQ casts to u_32_t.
> +			 */
>  			if (IP6_NEQ(&fin->fin_fi.fi_dst,
>  				    (i6addr_t *)&ip6->ip6_src))
>  				fin->fin_flx |= FI_BAD;
> Index: sys/dist/ipf/netinet/ip_fil.h
> ===================================================================
> RCS file: /cvsroot/src/sys/dist/ipf/netinet/ip_fil.h,v
> retrieving revision 1.6.12.4
> diff -u -p -r1.6.12.4 ip_fil.h
> --- sys/dist/ipf/netinet/ip_fil.h	16 Jul 2007 11:05:41 -0000	1.6.12.4
> +++ sys/dist/ipf/netinet/ip_fil.h	17 Jul 2007 14:39:00 -0000
> @@ -158,14 +158,14 @@ typedef	union	i6addr	{
>  #define	iplookupptr	vptr[0]
>  #define	iplookupfunc	lptr[1]
>  
> -#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	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])
>  
>  #define	IP6_EQ(a,b)	((I63(a) == I63(b)) && (I62(a) == I62(b)) && \
>  			 (I61(a) == I61(b)) && (I60(a) == I60(b)))