re: CVS commit: src/sys

[matthew green <mrg%eterna.com.au@localhost>]

Roy Marples writes:
> On 03/02/2021 21:45, David Young wrote:
> > On Wed, Feb 03, 2021 at 05:51:40AM +0000, Roy Marples wrote:
> >> Module Name:	src
> >> Committed By:	roy
> >> Date:		Wed Feb  3 05:51:40 UTC 2021
> >>
> >> Modified Files:
> >> 	src/sys/net: if_arp.h if_ether.h if_gre.h
> >> 	src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
> >> 	    ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> >>
> >> Log Message:
> >> Remove __packed from various network structures
> >>
> >> They are already network aligned and adding the __packed attribute
> >> just causes needless compiler warnings about accssing members of packed
> >> objects.
> > 
> > This change looks a little hasty to me.
> > 
> > It looks to me like some of these structs were __packed so that
> > they could be read/written directly from/to any offset in a packet
> > chain using mtod(), which does not pay any mind to the alignment
> > of `*t`:
> > 
> > #define mtod(m, t)      ((t)((m)->m_data))
> > 
> > I see gre_h is accessed in that way, just for one example.  I don't
> > see any reason in principle that every gre_h accessed through mtod()
> > should be 16-bit aligned in its buffer, but that is the alignment
> > the compiler will expect if gre_h is not __packed.  If the actual
> > alignment ever differs from compiler's expectation, then there
> > could be a bus error or an unwanted byte rotation/shift.
> > 
> > It looks to me like there's a bit of cleanup to do elsewhere before
> > removing __packed from network structures.
>
> ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both ends 
> seems to work fine. I also tested an amd64 endpoint.
>
> Not that I disagree with your assessment that the code can always be improved.

i looked at removing __packed from these when GCC 9 came
around and really started complaining about them.  however,
i was not able to convince myself that all the users were
actually safe if __packed was removed.

in particular, 'struct ip' has 4-byte objects at offset
14, 18, 22, and 26.  code accessing data directly from the
network may fail, and eg, mtod() makes it virtually
impossible to check for this at compile time.  sanitizers
could check at run time.

i really support the _idea_ of this change, but i'm pretty
far from convinced we're ready to make it for all these
types, and we won't know unles the code actually exercises
these paths (ie, we may find a crash in years in some error
path that uses misaligned accesses.)

removing __packed as much as possible, replacing it with
specific __aligned(n) where necessary, are both goals that
i'm strongly in favour of.  eg, we could perhaps avoid the
'struct ip' issue mentioned above if we mark both the
ip_src and ip_dst members as __aligned(2).

.. but i remain unsure and worried about this change
without significant testing or code reading.


.mrg.