From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ivan Skytte =?ISO-8859-1?Q?J=F8rgensen?= Date: Sun, 07 Jun 2020 21:35:19 +0000 Subject: Re: packed structures used in socket options Message-Id: <2213135.ChUyxVVRYb@isjsys> List-Id: References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: linux-sctp@vger.kernel.org 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 wrot= e: > > > > > > From: Michael Tuexen > > >> Sent: 07 June 2020 16:15 > > >>> On 7. Jun 2020, at 15:53, David Laight wr= ote: > > >>> > > >>> From: Michael Tuexen > > >>>> > > >>>> since gcc uses -Werror=ADdress-of-packed-member, I get warnings fo= r 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=3Dv5 > > >>>> .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 optio= n 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. >=20 > It depends on whether the rfc that describes the sockops says > the structures 'look like this' or 'contain the following members'. >=20 > > It should have been, but this was missed. Authors of all > > kernel implementation (FreeBSD, Linux, and Solaris) were involved. >=20 > Sounds like none of the right people even looked at it. I was involved. At that time (September 2005) the SCTP API was still evolvi= ng (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 rea= lized that the structures were different between 32bit and 64bit we had to = break the least number of programs, and the result were those ((packed)) st= ructs 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=F8rgensen wrote: > I followed the discussion in tsvwg mailing list. My interpretation of the= few=20 > answers is that this is a "quality of implementation issue" and that padd= ing=20 > fields are allowed but won't get into the RFC because it is an implementa= tion=20 > issue. So RFC6458 allows padding but doesn't list them. Incidentally, at that time (and perhaps still) sockaddr_storage had differe= nt alignement between 32-bit programs and 64-bit programs, and the multicas= t structures used in setsockopt() (group_req, group_source_req and group_f= ilter) had/has the same problem. /isj