| Message ID | 1484251157-245538-1-git-send-email-shannon.nelson@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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.
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 ?
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
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.
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.
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
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?
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 */ };
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(-)