tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

"Wire" definitions and __packed



Hello all,
a long time ago, Jason Thorpe added the packed attribute to a lot of
"wire" format definitions. Some of those like in_addr are part of a lot
of other interfaces that have nothing to do with on-wire
representations. The attribute has two functions:
(1) It removes any implicit padding as expected from the name.
(2) It also (unspectedly for many) removes any alignment constraints
from the structure.

The second item is problematic as can result in rather inefficient
byte-wise access. More important and the reason why I am bringing this
up: it impacts addresses to individual struct members. The current clang
development version has a new warning to trigger for situations like

   struct foo {
     int x;
   } __packed;

   int *f(struct foo *f) {
     return &f.x;
   }

The return value of f() is not guarenteed to be aligned correctly, so
this is effectively an implicit cast that raises the alignment. The
clang warning is still being refined to avoid triggering in situation
where alignment is explicitly not relevant (cast to intptr_t, cast to
char *, cast to void *) or where other alignment constraints are still
in place. Nevertheless quite a few cases arround the network stack
remain, triggering this warning. While investigating them, I have found
two different situations. Some places are safe and there should not have
been a __packed in first place. in_addr is a good example where the
attribute servers no real purpose. For other places it is not easy to
see where the alignment guarantees come from, so whether they work on
strict alignment platforms is difficult to tell.

I'd like to addressing this by cutting down on the first set. For this
purpose, I want to replace many of the __packed attributes in the
current network headers with CTASSERT of the proper size, especially for
those structs that are clearly not wire definitions by themselve. There
is a certain chance that some code path in the kernel currently does
depend on the implicit byte access to work, so this is not without risk.
On the other hand, it can result in more efficient code and better
diagnostic of mismatches, so IMO it will be an over all win. It should
also make it easier to identify cases where we do mishandled unaligned
access. Comments?

Joerg


Home | Main Index | Thread Index | Old Index