Re: packed structures used in socket options

[Michael Tuexen <Michael.Tuexen@lurchi.franken.de>]

On 7. Jun 2020, at 23:35, Ivan Skytte Jørgensen <isj-sctp@i1.dk> wrote:
> 
> On Sunday, 7 June 2020 22:21:41 CEST you wrote:
>> From: Michael Tuexen
>>> Sent: 07 June 2020 18:24
>>>> On 7. Jun 2020, at 19:14, David Laight <David.Laight@ACULAB.COM> wrote:
>>>> 
>>>> From: Michael Tuexen <Michael.Tuexen@lurchi.franken.de>
>>>>> Sent: 07 June 2020 16:15
>>>>>> On 7. Jun 2020, at 15:53, David Laight <David.Laight@ACULAB.COM> wrote:
>>>>>> 
>>>>>> From: Michael Tuexen
>>>>>>> 
>>>>>>> since gcc uses -Werror­dress-of-packed-member, I get warnings for my variant
>>>>>>> of packetdrill, which supports SCTP.
>>>>>>> 
>>>>>>> Here is why:
>>>>>>> 
>>>>>>> 
>>>>> 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/sctp.h?h=v5
>>>>>>> .7
>>>>>>> contains:
>>>>>>> 
>>>>>>> struct sctp_paddrparams {
>>>>>>> 	sctp_assoc_t		spp_assoc_id;
>>>>>>> 	struct sockaddr_storage	spp_address;
>>>>>>> 	__u32			spp_hbinterval;
>>>>>>> 	__u16			spp_pathmaxrxt;
>>>>>>> 	__u32			spp_pathmtu;
>>>>>>> 	__u32			spp_sackdelay;
>>>>>>> 	__u32			spp_flags;
>>>>>>> 	__u32			spp_ipv6_flowlabel;
>>>>>>> 	__u8			spp_dscp;
>>>>>>> } __attribute__((packed, aligned(4)));
>>>>>>> 
>>>>>>> This structure is only used in the IPPROTO_SCTP level socket option SCTP_PEER_ADDR_PARAMS.
>>>>>>> Why is it packed?
>>>>>> 
>>>>>> I'm guessing 'to remove holes to avoid leaking kernel data'.
>>>>>> 
>>>>>> The sctp socket api defines loads of structures that will have
>>>>>> holes in them if not packed.
>>>>> 
>>>>> Hi David,
>>>>> I agree that they have holes and we should have done better. The
>>>>> kernel definitely should also not leak kernel data. However, the
>>>>> way to handle this shouldn't be packing. I guess it is too late
>>>>> to change this?
>>>> 
>>>> Probably too late.
>>>> I've no idea how it got through the standards body either.
>>>> In fact, the standard may actually require the holes.
>>> 
>>> No, it does not. Avoiding holes was not taken into account.
>> 
>> It depends on whether the rfc that describes the sockops says
>> the structures 'look like this' or 'contain the following members'.
>> 
>>> It should have been, but this was missed. Authors of all
>>> kernel implementation (FreeBSD, Linux, and Solaris) were involved.
>> 
>> Sounds like none of the right people even looked at it.
> 
> I was involved. At that time (September 2005) the SCTP API was still evolving (first finalized in 2011), and one of the major users of the API was 32-bit programs running on 64-bit kernel (on powerpc as I recall). When we realized that the structures were different between 32bit and 64bit we had to break the least number of programs, and the result were those ((packed)) structs so 32-bit programs wouldn't be broken and we didn't need a xxx_compat translation layer in the kernel.
Ahh, I see. Thanks for the explanation.
> 
> I don't have access to TSVWG mailing list archive that far back but I found I wrote this summary here:
> 
> On Sunday, 25 September 2005 21:36:05 CEST Ivan Skytte Jørgensen wrote:
>> I followed the discussion in tsvwg mailing list. My interpretation of the few 
>> answers is that this is a "quality of implementation issue" and that padding 
>> fields are allowed but won't get into the RFC because it is an implementation 
>> issue.
> 
> 
> So RFC6458 allows padding but doesn't list them.
Yepp, that is my understanding (being a co-author).
> 
> 
> Incidentally, at that time (and perhaps still) sockaddr_storage had different alignement between 32-bit programs and 64-bit programs, and the multicast structures used in setsockopt() (group_req, group_source_req and  group_filter) had/has the same problem.
If I remember it correctly, I tested (FreeBSD) stuff on Little Endian, Big Endian, 32-bit, 64-bit, but not
run one binary on another platform.

Best regards
Michael
> 
> 
> /isj
> 
> 
>