[PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc

[PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc

[Shannon Nelson]

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 <shannon.nelson@oracle.com>
---
 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;
 	bool	exp;	/* In RFC6994 experimental option format */
 };
 
-- 
1.7.1

Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc

[Eric Dumazet]

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 <shannon.nelson@oracle.com>
> ---
>  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;
>  	bool	exp;	/* 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

[Rob Gardner]

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 <shannon.nelson@oracle.com>
>> ---
>>   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;
>>   	bool	exp;	/* 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

[Eric Dumazet]

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

[Shannon Nelson]

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

[David Miller]

From: Shannon Nelson <shannon.nelson@oracle.com>
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

[Shannon Nelson]

On 1/12/2017 12:41 PM, David Miller wrote:
> From: Shannon Nelson <shannon.nelson@oracle.com>
> 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

[David Miller]

From: Shannon Nelson <shannon.nelson@oracle.com>
Date: Thu, 12 Jan 2017 12:56:08 -0800

> 
> 
> On 1/12/2017 12:41 PM, David Miller wrote:
>> From: Shannon Nelson <shannon.nelson@oracle.com>
>> 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

[Eric Dumazet]

On Thu, 2017-01-12 at 16:18 -0500, David Miller wrote:
> From: Shannon Nelson <shannon.nelson@oracle.com>
> Date: Thu, 12 Jan 2017 12:56:08 -0800
> 
> > 
> > 
> > On 1/12/2017 12:41 PM, David Miller wrote:
> >> From: Shannon Nelson <shannon.nelson@oracle.com>
> >> 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];
        bool    exp;    /* In RFC6994 experimental option format */
 };
 

Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
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

[Shannon Nelson]

On 1/12/2017 1:47 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> 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

[David Miller]

From: Shannon Nelson <shannon.nelson@oracle.com>
Date: Thu, 12 Jan 2017 13:58:10 -0800

> 
> 
> On 1/12/2017 1:47 PM, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> 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.

Re: [PATCH] tcp: fix tcp_fastopen unaligned access complaints on sparc

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
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.