From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: Qian Cai <cai@lca.pw>
Cc: vyasevich@gmail.com, nhorman@tuxdriver.com,
linux-sctp@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net/sctp: fix GCC8+ -Wpacked-not-aligned warnings
Date: Sun, 28 Jul 2019 19:41:22 +0000 [thread overview]
Message-ID: <20190728194122.GN6204@localhost.localdomain> (raw)
In-Reply-To: <4A42819C-32E5-4869-8A3B-0D4F9DF1B53C@lca.pw>
On Sun, Jul 28, 2019 at 01:05:25PM -0400, Qian Cai wrote:
>
>
> > On Jul 26, 2019, at 5:30 PM, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> >
> > On Fri, Jul 26, 2019 at 04:57:39PM -0400, Qian Cai wrote:
> >> There are a lot of those warnings with GCC8+ 64bit,
> >>
> >> In file included from ./include/linux/sctp.h:42,
> >> from net/core/skbuff.c:47:
> >> ./include/uapi/linux/sctp.h:395:1: warning: alignment 4 of 'struct
> >> sctp_paddr_change' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:728:1: warning: alignment 4 of 'struct
> >> sctp_setpeerprim' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:727:26: warning: 'sspp_addr' offset 4 in
> >> 'struct sctp_setpeerprim' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage sspp_addr;
> >> ^~~~~~~~~
> >> ./include/uapi/linux/sctp.h:741:1: warning: alignment 4 of 'struct
> >> sctp_prim' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:740:26: warning: 'ssp_addr' offset 4 in
> >> 'struct sctp_prim' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage ssp_addr;
> >> ^~~~~~~~
> >> ./include/uapi/linux/sctp.h:792:1: warning: alignment 4 of 'struct
> >> sctp_paddrparams' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:784:26: warning: 'spp_address' offset 4 in
> >> 'struct sctp_paddrparams' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage spp_address;
> >> ^~~~~~~~~~~
> >> ./include/uapi/linux/sctp.h:905:1: warning: alignment 4 of 'struct
> >> sctp_paddrinfo' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:899:26: warning: 'spinfo_address' offset 4
> >> in 'struct sctp_paddrinfo' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage spinfo_address;
> >> ^~~~~~~~~~~~~~
> >> Fix them by aligning the structures and fields to 8 bytes instead.
> >>
> >> Signed-off-by: Qian Cai <cai@lca.pw>
> >> ---
> >> include/uapi/linux/sctp.h | 18 +++++++++---------
> >> 1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> >> index b8f2c4d56532..e7bd71cde882 100644
> >> --- a/include/uapi/linux/sctp.h
> >> +++ b/include/uapi/linux/sctp.h
> >
> > These changes gotta be more careful, if possible at all. As is, it's breaking UAPI.
>
> Could you please elaborate how this breaks userspace? I read through all the information
> I can find about UAPI and still have no clue. All I can see if that some field alignments changed
> which are expected, and it still take on 3 cachelines which should not hurt the performance.
For example on the struct I pointed below, if an application is
compiled using the old headers, sctp_paddrparams->spp_hbinterval is at
offset 132. But with this patch, it will change to 136. Then with a
kernel with this patch, it will look for the field at the wrong place.
>
> >
> > -before
> > +after patch
> >
> > struct sctp_paddrparams {
> > sctp_assoc_t spp_assoc_id; /* 0 4 */
> > - struct __kernel_sockaddr_storage spp_address __attribute__((__aligned__(1))); /* 4 128 */
> > - /* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */
> > - __u32 spp_hbinterval; /* 132 4 */
> > - __u16 spp_pathmaxrxt; /* 136 2 */
> > - __u32 spp_pathmtu; /* 138 4 */
> > - __u32 spp_sackdelay; /* 142 4 */
> > - __u32 spp_flags; /* 146 4 */
> > - __u32 spp_ipv6_flowlabel; /* 150 4 */
> > - __u8 spp_dscp; /* 154 1 */
> >
> > - /* size: 156, cachelines: 3, members: 9 */
> > + /* XXX 4 bytes hole, try to pack */
> > +
> > + struct __kernel_sockaddr_storage spp_address __attribute__((__aligned__(8))); /* 8 128 */
> > + /* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */
> > + __u32 spp_hbinterval; /* 136 4 */
> > + __u16 spp_pathmaxrxt; /* 140 2 */
> > + __u32 spp_pathmtu; /* 142 4 */
> > + __u32 spp_sackdelay; /* 146 4 */
> > + __u32 spp_flags; /* 150 4 */
> > + __u32 spp_ipv6_flowlabel; /* 154 4 */
> > + __u8 spp_dscp; /* 158 1 */
> > +
> > + /* size: 160, cachelines: 3, members: 9 */
> > + /* sum members: 155, holes: 1, sum holes: 4 */
> > /* padding: 1 */
> > - /* forced alignments: 1 */
> > - /* last cacheline: 28 bytes */
> > -} __attribute__((__aligned__(4)));
> > + /* forced alignments: 1, forced holes: 1, sum forced holes: 4 */
> > + /* last cacheline: 32 bytes */
> > +} __attribute__((__aligned__(8)));
> >
> >
> >> @@ -392,7 +392,7 @@ struct sctp_paddr_change {
> >> int spc_state;
> >> int spc_error;
> >> sctp_assoc_t spc_assoc_id;
> >> -} __attribute__((packed, aligned(4)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /*
> >> * spc_state: 32 bits (signed integer)
> >> @@ -724,8 +724,8 @@ struct sctp_assocparams {
> >> */
> >> struct sctp_setpeerprim {
> >> sctp_assoc_t sspp_assoc_id;
> >> - struct sockaddr_storage sspp_addr;
> >> -} __attribute__((packed, aligned(4)));
> >> + struct sockaddr_storage sspp_addr __attribute__((aligned(8)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /*
> >> * 7.1.10 Set Primary Address (SCTP_PRIMARY_ADDR)
> >> @@ -737,8 +737,8 @@ struct sctp_setpeerprim {
> >> */
> >> struct sctp_prim {
> >> sctp_assoc_t ssp_assoc_id;
> >> - struct sockaddr_storage ssp_addr;
> >> -} __attribute__((packed, aligned(4)));
> >> + struct sockaddr_storage ssp_addr __attribute__((aligned(8)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /* For backward compatibility use, define the old name too */
> >> #define sctp_setprim sctp_prim
> >> @@ -781,7 +781,7 @@ enum sctp_spp_flags {
> >>
> >> struct sctp_paddrparams {
> >> sctp_assoc_t spp_assoc_id;
> >> - struct sockaddr_storage spp_address;
> >> + struct sockaddr_storage spp_address __attribute__((aligned(8)));
> >> __u32 spp_hbinterval;
> >> __u16 spp_pathmaxrxt;
> >> __u32 spp_pathmtu;
> >> @@ -789,7 +789,7 @@ struct sctp_paddrparams {
> >> __u32 spp_flags;
> >> __u32 spp_ipv6_flowlabel;
> >> __u8 spp_dscp;
> >> -} __attribute__((packed, aligned(4)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /*
> >> * 7.1.18. Add a chunk that must be authenticated (SCTP_AUTH_CHUNK)
> >> @@ -896,13 +896,13 @@ struct sctp_stream_value {
> >> */
> >> struct sctp_paddrinfo {
> >> sctp_assoc_t spinfo_assoc_id;
> >> - struct sockaddr_storage spinfo_address;
> >> + struct sockaddr_storage spinfo_address __attribute__((aligned(8)));
> >> __s32 spinfo_state;
> >> __u32 spinfo_cwnd;
> >> __u32 spinfo_srtt;
> >> __u32 spinfo_rto;
> >> __u32 spinfo_mtu;
> >> -} __attribute__((packed, aligned(4)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /* Peer addresses's state. */
> >> /* UNKNOWN: Peer address passed by the upper layer in sendmsg or connect[x]
> >> --
> >> 1.8.3.1
>
WARNING: multiple messages have this Message-ID (diff)
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: Qian Cai <cai@lca.pw>
Cc: vyasevich@gmail.com, nhorman@tuxdriver.com,
linux-sctp@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net/sctp: fix GCC8+ -Wpacked-not-aligned warnings
Date: Sun, 28 Jul 2019 16:41:22 -0300 [thread overview]
Message-ID: <20190728194122.GN6204@localhost.localdomain> (raw)
In-Reply-To: <4A42819C-32E5-4869-8A3B-0D4F9DF1B53C@lca.pw>
On Sun, Jul 28, 2019 at 01:05:25PM -0400, Qian Cai wrote:
>
>
> > On Jul 26, 2019, at 5:30 PM, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> >
> > On Fri, Jul 26, 2019 at 04:57:39PM -0400, Qian Cai wrote:
> >> There are a lot of those warnings with GCC8+ 64bit,
> >>
> >> In file included from ./include/linux/sctp.h:42,
> >> from net/core/skbuff.c:47:
> >> ./include/uapi/linux/sctp.h:395:1: warning: alignment 4 of 'struct
> >> sctp_paddr_change' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:728:1: warning: alignment 4 of 'struct
> >> sctp_setpeerprim' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:727:26: warning: 'sspp_addr' offset 4 in
> >> 'struct sctp_setpeerprim' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage sspp_addr;
> >> ^~~~~~~~~
> >> ./include/uapi/linux/sctp.h:741:1: warning: alignment 4 of 'struct
> >> sctp_prim' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:740:26: warning: 'ssp_addr' offset 4 in
> >> 'struct sctp_prim' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage ssp_addr;
> >> ^~~~~~~~
> >> ./include/uapi/linux/sctp.h:792:1: warning: alignment 4 of 'struct
> >> sctp_paddrparams' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:784:26: warning: 'spp_address' offset 4 in
> >> 'struct sctp_paddrparams' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage spp_address;
> >> ^~~~~~~~~~~
> >> ./include/uapi/linux/sctp.h:905:1: warning: alignment 4 of 'struct
> >> sctp_paddrinfo' is less than 8 [-Wpacked-not-aligned]
> >> } __attribute__((packed, aligned(4)));
> >> ^
> >> ./include/uapi/linux/sctp.h:899:26: warning: 'spinfo_address' offset 4
> >> in 'struct sctp_paddrinfo' isn't aligned to 8 [-Wpacked-not-aligned]
> >> struct sockaddr_storage spinfo_address;
> >> ^~~~~~~~~~~~~~
> >> Fix them by aligning the structures and fields to 8 bytes instead.
> >>
> >> Signed-off-by: Qian Cai <cai@lca.pw>
> >> ---
> >> include/uapi/linux/sctp.h | 18 +++++++++---------
> >> 1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> >> index b8f2c4d56532..e7bd71cde882 100644
> >> --- a/include/uapi/linux/sctp.h
> >> +++ b/include/uapi/linux/sctp.h
> >
> > These changes gotta be more careful, if possible at all. As is, it's breaking UAPI.
>
> Could you please elaborate how this breaks userspace? I read through all the information
> I can find about UAPI and still have no clue. All I can see if that some field alignments changed
> which are expected, and it still take on 3 cachelines which should not hurt the performance.
For example on the struct I pointed below, if an application is
compiled using the old headers, sctp_paddrparams->spp_hbinterval is at
offset 132. But with this patch, it will change to 136. Then with a
kernel with this patch, it will look for the field at the wrong place.
>
> >
> > -before
> > +after patch
> >
> > struct sctp_paddrparams {
> > sctp_assoc_t spp_assoc_id; /* 0 4 */
> > - struct __kernel_sockaddr_storage spp_address __attribute__((__aligned__(1))); /* 4 128 */
> > - /* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */
> > - __u32 spp_hbinterval; /* 132 4 */
> > - __u16 spp_pathmaxrxt; /* 136 2 */
> > - __u32 spp_pathmtu; /* 138 4 */
> > - __u32 spp_sackdelay; /* 142 4 */
> > - __u32 spp_flags; /* 146 4 */
> > - __u32 spp_ipv6_flowlabel; /* 150 4 */
> > - __u8 spp_dscp; /* 154 1 */
> >
> > - /* size: 156, cachelines: 3, members: 9 */
> > + /* XXX 4 bytes hole, try to pack */
> > +
> > + struct __kernel_sockaddr_storage spp_address __attribute__((__aligned__(8))); /* 8 128 */
> > + /* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */
> > + __u32 spp_hbinterval; /* 136 4 */
> > + __u16 spp_pathmaxrxt; /* 140 2 */
> > + __u32 spp_pathmtu; /* 142 4 */
> > + __u32 spp_sackdelay; /* 146 4 */
> > + __u32 spp_flags; /* 150 4 */
> > + __u32 spp_ipv6_flowlabel; /* 154 4 */
> > + __u8 spp_dscp; /* 158 1 */
> > +
> > + /* size: 160, cachelines: 3, members: 9 */
> > + /* sum members: 155, holes: 1, sum holes: 4 */
> > /* padding: 1 */
> > - /* forced alignments: 1 */
> > - /* last cacheline: 28 bytes */
> > -} __attribute__((__aligned__(4)));
> > + /* forced alignments: 1, forced holes: 1, sum forced holes: 4 */
> > + /* last cacheline: 32 bytes */
> > +} __attribute__((__aligned__(8)));
> >
> >
> >> @@ -392,7 +392,7 @@ struct sctp_paddr_change {
> >> int spc_state;
> >> int spc_error;
> >> sctp_assoc_t spc_assoc_id;
> >> -} __attribute__((packed, aligned(4)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /*
> >> * spc_state: 32 bits (signed integer)
> >> @@ -724,8 +724,8 @@ struct sctp_assocparams {
> >> */
> >> struct sctp_setpeerprim {
> >> sctp_assoc_t sspp_assoc_id;
> >> - struct sockaddr_storage sspp_addr;
> >> -} __attribute__((packed, aligned(4)));
> >> + struct sockaddr_storage sspp_addr __attribute__((aligned(8)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /*
> >> * 7.1.10 Set Primary Address (SCTP_PRIMARY_ADDR)
> >> @@ -737,8 +737,8 @@ struct sctp_setpeerprim {
> >> */
> >> struct sctp_prim {
> >> sctp_assoc_t ssp_assoc_id;
> >> - struct sockaddr_storage ssp_addr;
> >> -} __attribute__((packed, aligned(4)));
> >> + struct sockaddr_storage ssp_addr __attribute__((aligned(8)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /* For backward compatibility use, define the old name too */
> >> #define sctp_setprim sctp_prim
> >> @@ -781,7 +781,7 @@ enum sctp_spp_flags {
> >>
> >> struct sctp_paddrparams {
> >> sctp_assoc_t spp_assoc_id;
> >> - struct sockaddr_storage spp_address;
> >> + struct sockaddr_storage spp_address __attribute__((aligned(8)));
> >> __u32 spp_hbinterval;
> >> __u16 spp_pathmaxrxt;
> >> __u32 spp_pathmtu;
> >> @@ -789,7 +789,7 @@ struct sctp_paddrparams {
> >> __u32 spp_flags;
> >> __u32 spp_ipv6_flowlabel;
> >> __u8 spp_dscp;
> >> -} __attribute__((packed, aligned(4)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /*
> >> * 7.1.18. Add a chunk that must be authenticated (SCTP_AUTH_CHUNK)
> >> @@ -896,13 +896,13 @@ struct sctp_stream_value {
> >> */
> >> struct sctp_paddrinfo {
> >> sctp_assoc_t spinfo_assoc_id;
> >> - struct sockaddr_storage spinfo_address;
> >> + struct sockaddr_storage spinfo_address __attribute__((aligned(8)));
> >> __s32 spinfo_state;
> >> __u32 spinfo_cwnd;
> >> __u32 spinfo_srtt;
> >> __u32 spinfo_rto;
> >> __u32 spinfo_mtu;
> >> -} __attribute__((packed, aligned(4)));
> >> +} __attribute__((packed, aligned(8)));
> >>
> >> /* Peer addresses's state. */
> >> /* UNKNOWN: Peer address passed by the upper layer in sendmsg or connect[x]
> >> --
> >> 1.8.3.1
>
next prev parent reply other threads:[~2019-07-28 19:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-26 20:57 [PATCH] net/sctp: fix GCC8+ -Wpacked-not-aligned warnings Qian Cai
2019-07-26 20:57 ` Qian Cai
2019-07-26 21:30 ` Marcelo Ricardo Leitner
2019-07-26 21:30 ` Marcelo Ricardo Leitner
2019-07-28 17:05 ` Qian Cai
2019-07-28 17:05 ` Qian Cai
2019-07-28 19:41 ` Marcelo Ricardo Leitner [this message]
2019-07-28 19:41 ` Marcelo Ricardo Leitner
2019-07-29 10:39 ` David Laight
2019-07-29 15:16 ` Qian Cai
2019-07-29 15:16 ` Qian Cai
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=20190728194122.GN6204@localhost.localdomain \
--to=marcelo.leitner@gmail.com \
--cc=cai@lca.pw \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=nhorman@tuxdriver.com \
--cc=vyasevich@gmail.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.