Re: CVS commit: src/sys

[David Young <dyoung%pobox.com@localhost>]

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.

Dave

-- 
David Young
dyoung%pobox.com@localhost    Urbana, IL    (217) 721-9981