On 04/02/2021 20:18, matthew green wrote:
Roy Marples writes:
On 03/02/2021 21:45, David Young wrote:
On Wed, Feb 03, 2021 at 05:51:40AM +, 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.
Isn't that already solved here?
http://anonhg.netbsd.org/src/rev/9f66cecd950e
And if I'm not wrong, just ignoring the warning causes alignment failures as it
stands? So it's just swapping one bad thing with another? If so, then there is
no right answer other than doing similar alignment checks for our mbufs.
Roy
