Re: CVS commit: src/sys
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. Dave -- David Young dyo...@pobox.comUrbana, IL(217) 721-9981
Re: CVS commit: src/sys
On 03.02.2021 16:52, Roy Marples wrote: > But you *have* to interogate the headers in order to work this out. I noted how this could break. I'm right now not affected by this myself. Please monitor gnats for reports. I defer any decisions and discussions (if any) now to others.
Re: CVS commit: src/sys/arch/hppa/gsc
Hello, On Wed, 3 Feb 2021 15:13:49 + "Tetsuya Isaki" wrote: > Module Name: src > Committed By: isaki > Date: Wed Feb 3 15:13:49 UTC 2021 > > Modified Files: > src/sys/arch/hppa/gsc: harmony.c > > Log Message: > Fix locking against myself. > trigger_output will be called with sc_intr_lock held. > From source code review, not tested. I just tested this on my C200 - playback works. The sample rate seems off - everything plays too slow, but that's probably completely unrelated. have fun Michael
Re: CVS commit: src/sys
On 03/02/2021 12:54, Kamil Rytarowski wrote:
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.
I don't buy this argument as we frequently do this:
r1.1
struct aaa {
uint16 a;
};
r1.2
struct oaaa {
uint16 a;
};
struct aaa {
uint16 a;
uint8 b;
};
Now if you choose to put your own stuff before and after in another struct,
frankly you are really on your own.
Here's another example:
struct ehteripudp {
struct etherhdr;
struct iphdr;
struct udphdr;
};
This still works!
However, it's potentially broken as you are making the assumption there are no
options after the ip header.
Infact this is how all the network headers have been designed.
From the start of the structure you can chain header structures together until
you either reach options or data.
But you *have* to interogate the headers in order to work this out.
Roy
Re: CVS commit: src/sys
On Wed, Feb 03, 2021 at 11:03:25AM +0100, 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. It doesn't break any of *our* ABI contracts. I also refuse us to be taken hostage here for any (hypothetical) user program exposing internals of the network stack on ABI boundaries. There is a very real price for the __packed and *any* fix for the misuse has the same ABI impacts. Joerg
Re: CVS commit: src/sys
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
>> #include
>> #include
>> #include
>>
>> 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
Re: CVS commit: src/sys
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
#include
#include
#include
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.
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.
Roy
Re: CVS commit: src/sys
On 03/02/2021 08:34, Joerg Sonnenberger 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. Please add a compile-time assert at least for _KERNEL that the size matches the expectation in that case. Done
Re: CVS commit: src/sys
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
#include
#include
#include
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;
};
int
main()
{
printf("sizeof(aaa2) = %zu, alignof(aaa2) = %zu\n",
sizeof(struct aaa2), alignof(struct aaa2));
printf("sizeof(bbb2) = %zu, alignof(bbb2) = %zu\n",
sizeof(struct bbb2), alignof(struct bbb2));
}
$ ./a.out
sizeof(aaa) = 4, alignof(aaa) = 1
sizeof(bbb) = 4, alignof(bbb) = 2
sizeof(aaa2) = 5, alignof(aaa2) = 1
sizeof(bbb2) = 6, alignof(bbb2) = 2
Please try t access these members of the structs with memcpy(3),
memcmp(3), memset(3) etc rather than directly with * or [].
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.
Re: CVS commit: src/sys
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. Please add a compile-time assert at least for _KERNEL that the size matches the expectation in that case. Joerg
