Re: CVS commit: src/sys

[Kamil Rytarowski <kamil%netbsd.org@localhost>]

On 03.02.2021 13:39, Roy Marples wrote:
> On 03/02/2021 10:03, Kamil Rytarowski wrote:
>> On 03.02.2021 06:51, 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 changes the ABI for userland programs. With __packed, these
>> structures, whenever contain at least 2-byte data, can be placed in a
>> different location inside another structure now.
>>
>> #include <sys/cdefs.h>
>> #include <stdio.h>
>> #include <stdint.h>
>> #include <stdalign.h>
>>
>> struct aaa {
>>          uint16_t a;
>>          uint8_t b;
>>          uint8_t c;
>> } __packed;
>>
>> struct aaa2 {
>>          uint8_t x;
>>          struct aaa y;
>> };
>>
>> struct bbb {
>>          uint16_t a;
>>          uint8_t b;
>>          uint8_t c;
>> };
>>
>> struct bbb2 {
>>          uint8_t x;
>>          struct bbb y;
>> };
> 
> Assuming that struct aaa and bbb are from NetBSD headers and aaa2 and
> bbb2 are your own constructs then you just have yourself to blame.
> 

It is a valid code to contain packed data in a structure without the
packed attribute.

> struct bbb2 {
>           uint8_t x;
>           struct bbb y;
> } __packed;
> 
> Makes bbb2 the same as aaa2.
> 
>> Before I saw your commit, I wanted to ask to revert the following
>> changes:
>>
>> icmp6: Remove __packed attribute from icmp6 structures
>>
>> https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462
>>
>>
>> ip6: Remove __packed attribute from ip6 structures
>>
>> https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f
>>
>>
>> The fallout can be subtle and hard to debug. Once, I removed __packed
>> from one innocent networking structure myself, qemu networking stopped
>> working at all.
> 
> How you use the structure is up to you.
> For the record, we were the only BSD to ever apply __packed to these
> structures and thanks to modern compilers emitting these wonderful
> warnings it proved to be a bad move.
> 

This is still a valid usage and ABI breakage for userland. You cannot
blame a user for using system structures and headers that stop working
after an upgrade, at least before at least libc version bump.

For the record, I broke ABI here (it was the reverse situation, addition
of __packed).

https://github.com/NetBSD/src/commit/a833bd5cfdba983ecb5560512a3547f46f35f11e

I vote to revert and handling these structures with appropriate
functions that are aware of potentially misaligned data operations.

If we you or the project resist and insists on ABI breakage, it should
be boldly documented.

> Roy