Date: Thu, 16 Jun 2005 22:02:28 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Marcel Moolenaar <marcel@xcllnt.net> Cc: freebsd-net@freebsd.org, Marcel Moolenaar <marcel@freebsd.org>, freebsd-pf@freebsd.org Subject: Re: ia64/81284: Unaligned Reference with pf on 5.4/IA64 Message-ID: <20050616210944.Y833@delplex.bde.org> In-Reply-To: <df7cf0532ee7aa367a11ff455ffecae1@xcllnt.net> References: <200506132123.j5DLNove069255@freefall.freebsd.org> <20050615204232.GX8526@insomnia.benzedrine.cx> <df7cf0532ee7aa367a11ff455ffecae1@xcllnt.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 15 Jun 2005, Marcel Moolenaar wrote:
> On Jun 15, 2005, at 1:42 PM, Daniel Hartmeier wrote:
>> If I understand the problem correctly, there is an underlying
>> network-generic question I'd like to ask here.
>>
>> When a function in the kernel gets passed a struct ip pointer, can it
>> assume that the struct ip object pointed to is properly aligned? Or
>> should it assume that this is not the case, and extract members more
>> carefully?
Yes. A struct ip pointer points, by definition, to a struct ip. All
structs are sufficiently aligned in C.
> That entirely depends. If a struct ip pointer is constructed without
> any form of casting, then one can assume that alignment is guaranteed.
> The compiler guarantees to do so, except of course in this case:
> the structure is defined as a packed structure. We, as the developers,
> have told the compiler to *NOT* guarantee alignment of fields. We're
> on our own and we miserably fail being on our own.
This case is not really different. Packed structs are, by definition,
still structs. All structs are sufficiently aligned in C. "Sufficiently"
just means that valid accesses to the struct are sufficiently aligned.
For arches with strict alignment requirements like ia64, this means
laboriously accessing the struct 1 byte at a time and combining the
results. So declaring a struct as packed when it doesn't need to be,
is mostly just a pessimization. For struct ip, this pessimization was
implemented in rev.1.20 of netinet/ip.h.
>> We can fix the access in pf of course, but if other functions rightfully
>> count on struct ip objects being properly aligned, this might simply
>> crash outside of pf, too.
I suppose the problems are that:
- pf makes invalid accesses like:
void foo(struct in_addr *iap); /* use *iap */
struct ip *ip;
foo(&ip->ip_src);
- the compiler doesn't diagnose the invalid access in the above. Since
*ip is packed, ip->ip_src in it isn't actually a struct in_addr -- it
is a packed struct in_addr, so it might not have sufficent alignment
to be a struct in_addr.
> True. But since struct ip is defined as packed, nobody can assume proper
> alignment of multi-byte fields and all code needs to be fixed if such
> assumptions are being made.
This is a reason why packed structs should never be used. Programming is
too hard if &ip->ip_src doesn't work, and even if it did work it would
export the pessimal packing.
>> If ia64 is different from other 64-bit
>> architectures (of which I only know amd64, sparc64 and alpha), please
>> explain what alignment rules there are for u_int32_t.
>
> ia64 is not different in this respect. That's why the bug is not
> specific to ia64. Note that amd64 may not be a perfect reference
> in this case because it's too much like i386, which does unaligned
> loads and stores.
ia64 is different in that it actually traps misaligned accesses.
i486+'s can trap all misaligned accesses too, at least in user mode, but
alignment checking is rarely turned on and gcc has never supported it.
E.g., gcc has always copying structs like "struct foo { short x[16]; };".
This struct is only guaranteed to be 16-bit aligned, but gcc now uses
only 32-bit accesses to copy it. So it is an ABI requirement in practice
at least for alignment checking to not be enabled. gcc also exploits
this to not generate different code to access struct ip's when struct ip is
bogusly packed.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050616210944.Y833>