packed structures used in socket options

packed structures used in socket options

[Michael Tuexen]

Dear all,

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?

Accessing spp_address, spp_pathmtu, spp_sackdelay, spp_flags, and spp_ipv6_flowlabel
results in warnings related to -Werror­dress-of-packed-member.

Am I supposed to access these fields via memcpy only?

Please note that a similar problem exists also with other structures in that file, I
used struct sctp_paddrparams as an example.

Best regards
Michael

RE: packed structures used in socket options

[David Laight]

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.

OTOH they shouldn't have been packed either.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: packed structures used in socket options

[Michael Tuexen]

> 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?

This means the corresponding fields can only be accessed via
memcpy() or one needs to tolerate unaligned access. Dealing with
warnings is one thing, but do you know if Linux supports
unaligned access on all platforms it supports (I'm not familiar
with enough with Linux)?

Best regards
Michael
> 
> OTOH they shouldn't have been packed either.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

RE: packed structures used in socket options

[David Laight]

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.

>  This means the corresponding fields can only be accessed via
> memcpy() or one needs to tolerate unaligned access. Dealing with
> warnings is one thing, but do you know if Linux supports
> unaligned access on all platforms it supports (I'm not familiar
> with enough with Linux)?

The compiler will generate loads shifts and ors to access misaligned
data on architectures like sparc where the hardware (quite reasonably)
doesn't allow them.
The code is horrid and you don't want to do it if it is avoidable.
But it is better that taking a fault and then emulating it.
That really shouldn't have even been allowed to happen.

I do remember a classic statement in a header from AT&T.
'All the fields are declared 'long' to ensure the same alignment
on all machines.'
AT&T had a protocol that passed the structure between machines
(without allowing for endianness).
I think it was SGI that did a 64bit interface using 'long'.
The TOG standard allowed the implementation to pick a type.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: packed structures used in socket options

[Michael Tuexen]

> 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 should have been, but this was missed. Authors of all
kernel implementation (FreeBSD, Linux, and Solaris) were involved.
> 
>> This means the corresponding fields can only be accessed via
>> memcpy() or one needs to tolerate unaligned access. Dealing with
>> warnings is one thing, but do you know if Linux supports
>> unaligned access on all platforms it supports (I'm not familiar
>> with enough with Linux)?
> 
> The compiler will generate loads shifts and ors to access misaligned
> data on architectures like sparc where the hardware (quite reasonably)
> doesn't allow them.
> The code is horrid and you don't want to do it if it is avoidable.
> But it is better that taking a fault and then emulating it.
OK, so there is no functional problem on Linux. The code will run
slower, but it will work. At some time using FreeBSD on arm, you would
read interesting values and on Sparc with Solaris you would get a core.

This means developers have to use  -Wno-address-of-packed-member when
compiling a program on Linux.

Best regards
Michael
> That really shouldn't have even been allowed to happen.
> 
> I do remember a classic statement in a header from AT&T.
> 'All the fields are declared 'long' to ensure the same alignment
> on all machines.'
> AT&T had a protocol that passed the structure between machines
> (without allowing for endianness).
> I think it was SGI that did a 64bit interface using 'long'.
> The TOG standard allowed the implementation to pick a type.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

RE: packed structures used in socket options

[David Laight]

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.

> >> This means the corresponding fields can only be accessed via
> >> memcpy() or one needs to tolerate unaligned access. Dealing with
> >> warnings is one thing, but do you know if Linux supports
> >> unaligned access on all platforms it supports (I'm not familiar
> >> with enough with Linux)?
> >
> > The compiler will generate loads shifts and ors to access misaligned
> > data on architectures like sparc where the hardware (quite reasonably)
> > doesn't allow them.
> > The code is horrid and you don't want to do it if it is avoidable.
> > But it is better that taking a fault and then emulating it.
>
> OK, so there is no functional problem on Linux. The code will run
> slower, but it will work. At some time using FreeBSD on arm, you would
> read interesting values and on Sparc with Solaris you would get a core.

Only if you try taking the addresses of the members.
If you access the structure members the compiler will avoid
faults on all architectures.

One problem with using 'packed' is that some applications may
use their own copies of the structures (even though they probably
shouldn't).
In that case the kernel will pull out the wrong fields.

> This means developers have to use  -Wno-address-of-packed-member when
> compiling a program on Linux.

I think that will only DTRT if the architecture handles misaligned
accesses (or the kernel emulates them).

Or don't write code that takes the addresses of the structure members.
The compiler warning is there for a purpose.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: packed structures used in socket options

[Ivan Skytte Jørgensen]

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.

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.


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.


/isj

Re: packed structures used in socket options

[Michael Tuexen]

> On 7. Jun 2020, at 22:21, David Laight <David.Laight@ACULAB.COM> 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 uses "is defined as"... Using "contain the following members" 
would have been a better way. But is wasn't used. So yes, we could
have minimised the number of holes. But also other structure have
them. So when passing them from kernel land to user land one has
to zero out the padding. Not optimal, but doable.
> 
>> 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.
Possible. At least the implementers were and it is an open process.
However, the rfc does not tell you to use packed structures.
So if you would follow the rfc, deal with the holes (which is
suboptimal, but doable), everything would be fine.
> 
>>>> This means the corresponding fields can only be accessed via
>>>> memcpy() or one needs to tolerate unaligned access. Dealing with
>>>> warnings is one thing, but do you know if Linux supports
>>>> unaligned access on all platforms it supports (I'm not familiar
>>>> with enough with Linux)?
>>> 
>>> The compiler will generate loads shifts and ors to access misaligned
>>> data on architectures like sparc where the hardware (quite reasonably)
>>> doesn't allow them.
>>> The code is horrid and you don't want to do it if it is avoidable.
>>> But it is better that taking a fault and then emulating it.
>> 
>> OK, so there is no functional problem on Linux. The code will run
>> slower, but it will work. At some time using FreeBSD on arm, you would
>> read interesting values and on Sparc with Solaris you would get a core.
> 
> Only if you try taking the addresses of the members.
> If you access the structure members the compiler will avoid
> faults on all architectures.
Ahh, OK. Thanks for the clarification.
> 
> One problem with using 'packed' is that some applications may
> use their own copies of the structures (even though they probably
> shouldn't).
> In that case the kernel will pull out the wrong fields.
Sure, if the kernel and userland don't agree on the structure,
that is a bad thing.
> 
>> This means developers have to use  -Wno-address-of-packed-member when
>> compiling a program on Linux.
> 
> I think that will only DTRT if the architecture handles misaligned
> accesses (or the kernel emulates them).
> 
> Or don't write code that takes the addresses of the structure members.
> The compiler warning is there for a purpose.
Sure. The point is that I wasn't expecting the structures to be packed,
since this is not specified in the rfc, not done in FreeBSD and Solaris.
And I do not see a reason for it (holes should be handled differently).
But I should have read the header file to be sure...

Best regards
Michael
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Re: packed structures used in socket options

[Michael Tuexen]

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
> 
> 
> 

RE: packed structures used in socket options

[David Laight]

From: Michael Tuexen
> Sent: 07 June 2020 22:51
> > On 7. Jun 2020, at 22:21, David Laight <David.Laight@ACULAB.COM> 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 uses "is defined as"... Using "contain the following members"
> would have been a better way. But is wasn't used. So yes, we could
> have minimised the number of holes. But also other structure have
> them. So when passing them from kernel land to user land one has
> to zero out the padding. Not optimal, but doable.

If it says 'defined as' then the 'packed' is just wrong.

No idea what can be done about it.
But an application is within its rights to define the structure
as it is defined in the rfc.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: packed structures used in socket options

[David Laight]

From: Ivan Skytte Jørgensen
> Sent: 07 June 2020 22:35
...
> > > >>>> 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 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.

I was also looking at all the __u16 in that header - borked.

Ok, so the intention was to avoid padding caused by the alignment
of sockaddr_storage rather than around the '__u16 spp_flags'.

I'd have to look up what (packed, aligned(4)) actually means.
It could force the structure to be fully packed (no holes)
but always have an overall alignment of 4.

It might have been clearer to put an 'aligned(4)' attribute
on the spp_address field itself.
Or even wonder whether sockaddr_storage should actually
have 8 byte alignment.

If it has 16 byte alignment then you cannot cast an IPv4
socket buffer address (which will be at most 4 byte aligned)
to sockaddr_storage and expect the compiler not to generate
code that will crash and burn on sparc64.

ISTR that the NetBSD view was that 'sockaddr_storage' should
never actually be instantiated - it only existed as a typed
pointer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: packed structures used in socket options

[Michael Tuexen]

> On 8. Jun 2020, at 18:18, David Laight <David.Laight@ACULAB.COM> wrote:
> 
> From: Ivan Skytte Jørgensen
>> Sent: 07 June 2020 22:35
> ...
>>>>>>>> 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 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.
> 
> I was also looking at all the __u16 in that header - borked.
> 
> Ok, so the intention was to avoid padding caused by the alignment
> of sockaddr_storage rather than around the '__u16 spp_flags'.
> 
> I'd have to look up what (packed, aligned(4)) actually means.
> It could force the structure to be fully packed (no holes)
> but always have an overall alignment of 4.
> 
> It might have been clearer to put an 'aligned(4)' attribute
> on the spp_address field itself.
> Or even wonder whether sockaddr_storage should actually
> have 8 byte alignment.
> 
> If it has 16 byte alignment then you cannot cast an IPv4
> socket buffer address (which will be at most 4 byte aligned)
> to sockaddr_storage and expect the compiler not to generate
> code that will crash and burn on sparc64.
> 
> ISTR that the NetBSD view was that 'sockaddr_storage' should
> never actually be instantiated - it only existed as a typed
> pointer.
Not sure this is correct. I would say this applies to stuct sockaddr *.
I have seen instantiated sockaddr_storage variable in generic code,
where you need to provide enough space to hold an address, not yet
knowing the address family. However, I'm not familiar with the NetBSD
code base.

Best regards
Michael
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

RE: packed structures used in socket options

[David Laight]

RnJvbTogTWljaGFlbCBUdWV4ZW4NCj4gU2VudDogMDggSnVuZSAyMDIwIDE4OjM3DQo+ID4gT24g
OC4gSnVuIDIwMjAsIGF0IDE4OjE4LCBEYXZpZCBMYWlnaHQgPERhdmlkLkxhaWdodEBBQ1VMQUIu
Q09NPiB3cm90ZToNCj4gPg0KPiA+IEZyb206IEl2YW4gU2t5dHRlIErDuHJnZW5zZW4NCj4gPj4g
U2VudDogMDcgSnVuZSAyMDIwIDIyOjM1DQo+ID4gLi4uDQo+ID4+Pj4+Pj4+IGNvbnRhaW5zOg0K
PiA+Pj4+Pj4+Pg0KPiA+Pj4+Pj4+PiBzdHJ1Y3Qgc2N0cF9wYWRkcnBhcmFtcyB7DQo+ID4+Pj4+
Pj4+IAlzY3RwX2Fzc29jX3QJCXNwcF9hc3NvY19pZDsNCj4gPj4+Pj4+Pj4gCXN0cnVjdCBzb2Nr
YWRkcl9zdG9yYWdlCXNwcF9hZGRyZXNzOw0KPiA+Pj4+Pj4+PiAJX191MzIJCQlzcHBfaGJpbnRl
cnZhbDsNCj4gPj4+Pj4+Pj4gCV9fdTE2CQkJc3BwX3BhdGhtYXhyeHQ7DQo+ID4+Pj4+Pj4+IAlf
X3UzMgkJCXNwcF9wYXRobXR1Ow0KPiA+Pj4+Pj4+PiAJX191MzIJCQlzcHBfc2Fja2RlbGF5Ow0K
PiA+Pj4+Pj4+PiAJX191MzIJCQlzcHBfZmxhZ3M7DQo+ID4+Pj4+Pj4+IAlfX3UzMgkJCXNwcF9p
cHY2X2Zsb3dsYWJlbDsNCj4gPj4+Pj4+Pj4gCV9fdTgJCQlzcHBfZHNjcDsNCj4gPj4+Pj4+Pj4g
fSBfX2F0dHJpYnV0ZV9fKChwYWNrZWQsIGFsaWduZWQoNCkpKTsNCj4gPj4+Pj4+Pj4NCj4gPj4+
Pj4+Pj4gVGhpcyBzdHJ1Y3R1cmUgaXMgb25seSB1c2VkIGluIHRoZSBJUFBST1RPX1NDVFAgbGV2
ZWwgc29ja2V0IG9wdGlvbiBTQ1RQX1BFRVJfQUREUl9QQVJBTVMuDQo+ID4+Pj4+Pj4+IFdoeSBp
cyBpdCBwYWNrZWQ/DQo+ID4gLi4uDQo+ID4+IEkgd2FzIGludm9sdmVkLiBBdCB0aGF0IHRpbWUg
KFNlcHRlbWJlciAyMDA1KSB0aGUgU0NUUCBBUEkgd2FzIHN0aWxsIGV2b2x2aW5nIChmaXJzdCBm
aW5hbGl6ZWQgaW4NCj4gPj4gMjAxMSksIGFuZCBvbmUgb2YgdGhlIG1ham9yIHVzZXJzIG9mIHRo
ZSBBUEkgd2FzIDMyLWJpdCBwcm9ncmFtcyBydW5uaW5nIG9uIDY0LWJpdCBrZXJuZWwgKG9uDQo+
IHBvd2VycGMNCj4gPj4gYXMgSSByZWNhbGwpLiBXaGVuIHdlIHJlYWxpemVkIHRoYXQgdGhlIHN0
cnVjdHVyZXMgd2VyZSBkaWZmZXJlbnQgYmV0d2VlbiAzMmJpdCBhbmQgNjRiaXQgd2UgaGFkIHRv
DQo+ID4+IGJyZWFrIHRoZSBsZWFzdCBudW1iZXIgb2YgcHJvZ3JhbXMsIGFuZCB0aGUgcmVzdWx0
IHdlcmUgdGhvc2UgKChwYWNrZWQpKSBzdHJ1Y3RzIHNvIDMyLWJpdCBwcm9ncmFtcw0KPiA+PiB3
b3VsZG4ndCBiZSBicm9rZW4gYW5kIHdlIGRpZG4ndCBuZWVkIGEgeHh4X2NvbXBhdCB0cmFuc2xh
dGlvbiBsYXllciBpbiB0aGUga2VybmVsLg0KPiA+DQo+ID4gSSB3YXMgYWxzbyBsb29raW5nIGF0
IGFsbCB0aGUgX191MTYgaW4gdGhhdCBoZWFkZXIgLSBib3JrZWQuDQo+ID4NCj4gPiBPaywgc28g
dGhlIGludGVudGlvbiB3YXMgdG8gYXZvaWQgcGFkZGluZyBjYXVzZWQgYnkgdGhlIGFsaWdubWVu
dA0KPiA+IG9mIHNvY2thZGRyX3N0b3JhZ2UgcmF0aGVyIHRoYW4gYXJvdW5kIHRoZSAnX191MTYg
c3BwX2ZsYWdzJy4NCj4gPg0KPiA+IEknZCBoYXZlIHRvIGxvb2sgdXAgd2hhdCAocGFja2VkLCBh
bGlnbmVkKDQpKSBhY3R1YWxseSBtZWFucy4NCj4gPiBJdCBjb3VsZCBmb3JjZSB0aGUgc3RydWN0
dXJlIHRvIGJlIGZ1bGx5IHBhY2tlZCAobm8gaG9sZXMpDQo+ID4gYnV0IGFsd2F5cyBoYXZlIGFu
IG92ZXJhbGwgYWxpZ25tZW50IG9mIDQuDQo+ID4NCj4gPiBJdCBtaWdodCBoYXZlIGJlZW4gY2xl
YXJlciB0byBwdXQgYW4gJ2FsaWduZWQoNCknIGF0dHJpYnV0ZQ0KPiA+IG9uIHRoZSBzcHBfYWRk
cmVzcyBmaWVsZCBpdHNlbGYuDQo+ID4gT3IgZXZlbiB3b25kZXIgd2hldGhlciBzb2NrYWRkcl9z
dG9yYWdlIHNob3VsZCBhY3R1YWxseQ0KPiA+IGhhdmUgOCBieXRlIGFsaWdubWVudC4NCj4gPg0K
PiA+IElmIGl0IGhhcyAxNiBieXRlIGFsaWdubWVudCB0aGVuIHlvdSBjYW5ub3QgY2FzdCBhbiBJ
UHY0DQo+ID4gc29ja2V0IGJ1ZmZlciBhZGRyZXNzICh3aGljaCB3aWxsIGJlIGF0IG1vc3QgNCBi
eXRlIGFsaWduZWQpDQo+ID4gdG8gc29ja2FkZHJfc3RvcmFnZSBhbmQgZXhwZWN0IHRoZSBjb21w
aWxlciBub3QgdG8gZ2VuZXJhdGUNCj4gPiBjb2RlIHRoYXQgd2lsbCBjcmFzaCBhbmQgYnVybiBv
biBzcGFyYzY0Lg0KDQpBY3R1YWxseSwgd2hhdCBoYXBwZW5zIHdoZW4gdGhlIG1pc2FsaWduZWQg
J3N0cnVjdCBzb2NrYWRkcicNCihpbiB0aGUgc2N0cCBvcHRpb25zKSBpcyBwYXNzZWQgdGhyb3Vn
aCB0byBhIGZ1bmN0aW9uDQp0aGF0IGV4cGVjdHMgaXQgdG8gYmUgYWxpZ25lZCBhbmQgdGhlbiBh
Y2Nlc3NlcyBwYXJ0IG9mIChzYXkpDQphbiBJUHY2IHN0cnVjdHVyZSB1c2luZyA4IGJ5dGVzIGFj
Y2Vzc2VzLg0KVGhhdCB3aWxsICdjcmFzaCBhbmQgYnVybicgb24gc3BhcmM2NCBhcyB3ZWxsLg0K
DQo+ID4gSVNUUiB0aGF0IHRoZSBOZXRCU0QgdmlldyB3YXMgdGhhdCAnc29ja2FkZHJfc3RvcmFn
ZScgc2hvdWxkDQo+ID4gbmV2ZXIgYWN0dWFsbHkgYmUgaW5zdGFudGlhdGVkIC0gaXQgb25seSBl
eGlzdGVkIGFzIGEgdHlwZWQNCj4gPiBwb2ludGVyLg0KPg0KPiBOb3Qgc3VyZSB0aGlzIGlzIGNv
cnJlY3QuIEkgd291bGQgc2F5IHRoaXMgYXBwbGllcyB0byBzdHVjdCBzb2NrYWRkciAqLg0KPiBJ
IGhhdmUgc2VlbiBpbnN0YW50aWF0ZWQgc29ja2FkZHJfc3RvcmFnZSB2YXJpYWJsZSBpbiBnZW5l
cmljIGNvZGUsDQo+IHdoZXJlIHlvdSBuZWVkIHRvIHByb3ZpZGUgZW5vdWdoIHNwYWNlIHRvIGhv
bGQgYW4gYWRkcmVzcywgbm90IHlldA0KPiBrbm93aW5nIHRoZSBhZGRyZXNzIGZhbWlseS4gSG93
ZXZlciwgSSdtIG5vdCBmYW1pbGlhciB3aXRoIHRoZSBOZXRCU0QNCj4gY29kZSBiYXNlLg0KDQpC
YXNpY2FsbHkgeW91IHNob3VsZCBhbHdheXMgaGF2ZSB0aGUgYWRkcmVzcyBsZW5ndGguDQpJIGp1
c3QgcmVtZW1iZXIgQ2hyaXN0b3MgY29tcGxhaW5pbmcgYWJvdXQgc29tZSBrZXJuZWwgY29kZQ0K
dGhhdCBhbGxvY2F0ZWQgb25lIG9uIHN0YWNrLg0KKE15IE5ldEJTRCAnY29tbWl0IGJpdCcgaGFz
IHJhdGhlciBsYXBzZWQuKQ0KDQoJRGF2aWQNCg0KLQ0KUmVnaXN0ZXJlZCBBZGRyZXNzIExha2Vz
aWRlLCBCcmFtbGV5IFJvYWQsIE1vdW50IEZhcm0sIE1pbHRvbiBLZXluZXMsIE1LMSAxUFQsIFVL
DQpSZWdpc3RyYXRpb24gTm86IDEzOTczODYgKFdhbGVzKQ0K