Mark Kettenis <mark.kette...@xs4all.nl> wrote:
> To be honest, I do think that adding __packed is a reasonable way to
> handle protocol structs like this where performance doesn't really
> matter. This translates into __attribute__((packed)) and both GCC and
> LLVM started treating that in a way to signal that the data might not
> be properly aligned and use byte access on architectures that need
> strict alignment. This is still not explicitly documented but I don't
> think the compiler writers can backtrack on that at this point.
__packed does not mean "use bytes". It means ensure there are no
padding bytes, and then on strict alignment systems when something is
misaligned, the compiler can see it must use smaller loads.
Unfortunately, tcpdump also parses encapsulated protocols, and some of
the outer layers are limited to short-alignment. So if an inner layer
has a 32-bit value inside the __packed array after packing, the compiler
will believe it is still on a 32-bit boundary, and not use char or short
accesses. This will fault, because tcpdump is passing a pointe to an
object with tighter alignment.
__packed is not saying "each object must be loaded as bytes", and I
really am susprised you claim that is what happens today. That is a
crazy expensive choice on some architectures.
So I think __packed is not enough for what tcpdump is doing.
Perhaps some of the compilers are being more cynical, or their costing
of loads leads them to do smaller-storage operations... but I have a
recollection that at least gcc on the alpha gets confused and does the
wrong thing.
> > Index: print-wg.c
> > ===================================================================
> > RCS file: src/usr.sbin/tcpdump/print-wg.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 print-wg.c
> > --- print-wg.c 14 Apr 2021 19:34:56 -0000 1.6
> > +++ print-wg.c 14 Sep 2021 12:42:32 -0000
> > @@ -34,20 +34,20 @@ struct wg_initiation {
> > uint32_t type;
> > uint32_t sender;
> > uint8_t fill[140]; /* Includes ephemeral + MAC */
> > -};
> > +} __packed;
> >
> > struct wg_response {
> > uint32_t type;
> > uint32_t sender;
> > uint32_t receiver;
> > uint8_t fill[80]; /* Includes ephemeral + MAC */
> > -};
> > +} __packed;
> >
> > struct wg_cookie {
> > uint32_t type;
> > uint32_t receiver;
> > uint8_t fill[56]; /* Includes nonce + encrypted cookie */
> > -};
> > +} __packed;
> >
> > struct wg_data {
> > uint32_t type;
> > @@ -55,7 +55,7 @@ struct wg_data {
> > uint64_t nonce;
> > /* uint8_t data[variable]; - Variable length data */
> > uint8_t mac[16];
> > -};
> > +} __packed;
> >
> > /*
> > * Check if packet is a WireGuard packet, as WireGuard may run on any port.
> >
> >
>
