linux-sctp.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ivan Skytte Jørgensen" <isj-sctp@i1.dk>
To: linux-sctp@vger.kernel.org
Subject: Re: packed structures used in socket options
Date: Sun, 07 Jun 2020 21:35:19 +0000	[thread overview]
Message-ID: <2213135.ChUyxVVRYb@isjsys> (raw)
In-Reply-To: <CBFEFEF1-127A-4ADA-B438-B171B9E26282@lurchi.franken.de>

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

  parent reply	other threads:[~2020-06-07 21:35 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-07 10:49 packed structures used in socket options Michael Tuexen
2020-06-07 13:53 ` David Laight
2020-06-07 15:15 ` Michael Tuexen
2020-06-07 17:14 ` David Laight
2020-06-07 17:23 ` Michael Tuexen
2020-06-07 20:21 ` David Laight
2020-06-07 21:35 ` Ivan Skytte Jørgensen [this message]
2020-06-08 16:18   ` David Laight
2020-06-08 17:37     ` Michael Tuexen
2020-06-08 21:13       ` David Laight
2020-06-07 21:51 ` Michael Tuexen
2020-06-08  8:17   ` David Laight
2020-06-07 21:55 ` Michael Tuexen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2213135.ChUyxVVRYb@isjsys \
    --to=isj-sctp@i1.dk \
    --cc=linux-sctp@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).