Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
On Thu, 2017-01-12 at 11:59 -0800, Shannon Nelson wrote:
> Fix up a data alignment issue on sparc by swapping the order
> of the cookie byte array field with the length field in
> struct tcp_fastopen_cookie
>
> This addresses log complaints like these:
> log_unaligned: 113 callbacks suppressed
> Kernel unaligned access at TPC[976490] tcp_try_fastopen+0x2d0/0x360
> Kernel unaligned access at TPC[9764ac] tcp_try_fastopen+0x2ec/0x360
> Kernel unaligned access at TPC[9764c8] tcp_try_fastopen+0x308/0x360
> Kernel unaligned access at TPC[9764e4] tcp_try_fastopen+0x324/0x360
> Kernel unaligned access at TPC[976490] tcp_try_fastopen+0x2d0/0x360
>
> Signed-off-by: Shannon Nelson
> ---
> include/linux/tcp.h |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index fc5848d..95cda75 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -62,8 +62,8 @@ static inline unsigned int tcp_optlen(const struct sk_buff
> *skb)
>
> /* TCP Fast Open Cookie as stored in memory */
> struct tcp_fastopen_cookie {
> - s8 len;
> u8 val[TCP_FASTOPEN_COOKIE_MAX];
> + s8 len;
> boolexp;/* In RFC6994 experimental option format */
> };
>
Strange... Do you have an explanation of why this patch would be
needed ? A compiler issue ?
s8 and u8 are bytes after all.
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
On 01/12/2017 01:13 PM, Eric Dumazet wrote:
On Thu, 2017-01-12 at 11:59 -0800, Shannon Nelson wrote:
Fix up a data alignment issue on sparc by swapping the order
of the cookie byte array field with the length field in
struct tcp_fastopen_cookie
This addresses log complaints like these:
log_unaligned: 113 callbacks suppressed
Kernel unaligned access at TPC[976490] tcp_try_fastopen+0x2d0/0x360
Kernel unaligned access at TPC[9764ac] tcp_try_fastopen+0x2ec/0x360
Kernel unaligned access at TPC[9764c8] tcp_try_fastopen+0x308/0x360
Kernel unaligned access at TPC[9764e4] tcp_try_fastopen+0x324/0x360
Kernel unaligned access at TPC[976490] tcp_try_fastopen+0x2d0/0x360
Signed-off-by: Shannon Nelson
---
include/linux/tcp.h |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index fc5848d..95cda75 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -62,8 +62,8 @@ static inline unsigned int tcp_optlen(const struct sk_buff
*skb)
/* TCP Fast Open Cookie as stored in memory */
struct tcp_fastopen_cookie {
- s8 len;
u8 val[TCP_FASTOPEN_COOKIE_MAX];
+ s8 len;
boolexp;/* In RFC6994 experimental option format */
};
Strange... Do you have an explanation of why this patch would be
needed ? A compiler issue ?
s8 and u8 are bytes after all.
I suspect that someplace, somebody is casting val to an int * or
something like that.
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
On Thu, 2017-01-12 at 13:15 -0700, Rob Gardner wrote: > > I suspect that someplace, somebody is casting val to an int * or > something like that. Then that would be the bug. Can we root cause this please ?
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
On 1/12/2017 12:25 PM, Eric Dumazet wrote: On Thu, 2017-01-12 at 13:15 -0700, Rob Gardner wrote: I suspect that someplace, somebody is casting val to an int * or something like that. Then that would be the bug. Can we root cause this please ? Look in net/ipv4/tcp_fastopen.c:tcp_fastopen_cookie_gen() for the line struct in6_addr *buf = (struct in6_addr *) tmp.val; sln
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
From: Eric Dumazet Date: Thu, 12 Jan 2017 12:25:33 -0800 > On Thu, 2017-01-12 at 13:15 -0700, Rob Gardner wrote: > >> >> I suspect that someplace, somebody is casting val to an int * or >> something like that. > > Then that would be the bug. Can we root cause this please ? The three accesses to foc->val are via function calls, at least when I try to build it, one via memcmp(), one via memcpy() (for the structure assignment at the end of the function) and one via a call into the crypto layer when we do tcp_fastopen_cookie_gen). So if the PC is inside of tcp_try_fastopen() it has to be something else, or something specific to your gcc and build.
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
From: Shannon Nelson Date: Thu, 12 Jan 2017 12:30:38 -0800 > On 1/12/2017 12:25 PM, Eric Dumazet wrote: >> On Thu, 2017-01-12 at 13:15 -0700, Rob Gardner wrote: >> >>> >>> I suspect that someplace, somebody is casting val to an int * or >>> something like that. >> >> Then that would be the bug. Can we root cause this please ? >> >> > > Look in net/ipv4/tcp_fastopen.c:tcp_fastopen_cookie_gen() for the line > >struct in6_addr *buf = (struct in6_addr *) tmp.val; Oh yeah, that's it. I didn't notice that at all.
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
On 1/12/2017 12:41 PM, David Miller wrote: From: Shannon Nelson Date: Thu, 12 Jan 2017 12:30:38 -0800 On 1/12/2017 12:25 PM, Eric Dumazet wrote: On Thu, 2017-01-12 at 13:15 -0700, Rob Gardner wrote: I suspect that someplace, somebody is casting val to an int * or something like that. Then that would be the bug. Can we root cause this please ? Look in net/ipv4/tcp_fastopen.c:tcp_fastopen_cookie_gen() for the line struct in6_addr *buf = (struct in6_addr *) tmp.val; Oh yeah, that's it. I didn't notice that at all. It looked to me like swapping the data fields would be the easiest and least impactive way to fix this. I didn't want to mess with the logic. I'm certainly open to other suggestions. sln
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
From: Shannon Nelson Date: Thu, 12 Jan 2017 12:56:08 -0800 > > > On 1/12/2017 12:41 PM, David Miller wrote: >> From: Shannon Nelson >> Date: Thu, 12 Jan 2017 12:30:38 -0800 >> >>> On 1/12/2017 12:25 PM, Eric Dumazet wrote: On Thu, 2017-01-12 at 13:15 -0700, Rob Gardner wrote: > > I suspect that someplace, somebody is casting val to an int * or > something like that. Then that would be the bug. Can we root cause this please ? >>> >>> Look in net/ipv4/tcp_fastopen.c:tcp_fastopen_cookie_gen() for the line >>> >>> struct in6_addr *buf = (struct in6_addr *) tmp.val; >> >> Oh yeah, that's it. I didn't notice that at all. >> > > It looked to me like swapping the data fields would be the easiest and > least impactive way to fix this. I didn't want to mess with the > logic. I'm certainly open to other suggestions. Given the nature of the problem, your fix is probably fine. Eric, any objections?
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
On Thu, 2017-01-12 at 16:18 -0500, David Miller wrote:
> From: Shannon Nelson
> Date: Thu, 12 Jan 2017 12:56:08 -0800
>
> >
> >
> > On 1/12/2017 12:41 PM, David Miller wrote:
> >> From: Shannon Nelson
> >> Date: Thu, 12 Jan 2017 12:30:38 -0800
> >>
> >>> On 1/12/2017 12:25 PM, Eric Dumazet wrote:
> On Thu, 2017-01-12 at 13:15 -0700, Rob Gardner wrote:
>
> >
> > I suspect that someplace, somebody is casting val to an int * or
> > something like that.
>
> Then that would be the bug. Can we root cause this please ?
>
>
> >>>
> >>> Look in net/ipv4/tcp_fastopen.c:tcp_fastopen_cookie_gen() for the line
> >>>
> >>>struct in6_addr *buf = (struct in6_addr *) tmp.val;
> >>
> >> Oh yeah, that's it. I didn't notice that at all.
> >>
> >
> > It looked to me like swapping the data fields would be the easiest and
> > least impactive way to fix this. I didn't want to mess with the
> > logic. I'm certainly open to other suggestions.
>
> Given the nature of the problem, your fix is probably fine.
>
> Eric, any objections?
I am still objecting to this fix.
gcc makes no provision for aligning an variable that has alignof() = 1
We had such issues in the past.
We need the proper annotation on ->val field itself, to get proper
alignment.
Then moving around the other field is a matter of avoiding a hole.
val should be an union, so that proper alignment is enforced by one
member.
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index
fc5848dad7a43216b3f124c4afdaa6b64b23910c..5b790abf4c16313c9110996683be3a7fb368b66f
100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -62,8 +62,13 @@ static inline unsigned int tcp_optlen(const struct sk_buff
*skb)
/* TCP Fast Open Cookie as stored in memory */
struct tcp_fastopen_cookie {
+ union {
+ u8 val[TCP_FASTOPEN_COOKIE_MAX];
+#if IS_ENABLED(CONFIG_IPV6)
+ struct in6_addr addr;
+#endif
+ };
s8 len;
- u8 val[TCP_FASTOPEN_COOKIE_MAX];
boolexp;/* In RFC6994 experimental option format */
};
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
From: Eric Dumazet Date: Thu, 12 Jan 2017 13:36:30 -0800 > val should be an union, so that proper alignment is enforced by one > member. Sure, annotating the type so that it is aligned correctly makes sense.
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
On 1/12/2017 1:47 PM, David Miller wrote: From: Eric Dumazet Date: Thu, 12 Jan 2017 13:36:30 -0800 val should be an union, so that proper alignment is enforced by one member. Sure, annotating the type so that it is aligned correctly makes sense. ... and we should change the offending pointer assignment as well. I can use this and respin a v2 if we're happy with this solution. sln
Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc
From: Shannon Nelson Date: Thu, 12 Jan 2017 13:58:10 -0800 > > > On 1/12/2017 1:47 PM, David Miller wrote: >> From: Eric Dumazet >> Date: Thu, 12 Jan 2017 13:36:30 -0800 >> >>> val should be an union, so that proper alignment is enforced by one >>> member. >> >> Sure, annotating the type so that it is aligned correctly makes >> sense. >> > > ... and we should change the offending pointer assignment as well. I > can use this and respin a v2 if we're happy with this solution. I am.
