NetBSD Problem Report #43541
From www@NetBSD.org Mon Jun 28 16:31:07 2010
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
by www.NetBSD.org (Postfix) with ESMTP id DEEB563BA69
for <gnats-bugs@gnats.NetBSD.org>; Mon, 28 Jun 2010 16:31:06 +0000 (UTC)
Message-Id: <20100628163106.71C6A63BA68@www.NetBSD.org>
Date: Mon, 28 Jun 2010 16:31:06 +0000 (UTC)
From: gandersen@cradlepoint.com
Reply-To: gandersen@cradlepoint.com
To: gnats-bugs@NetBSD.org
Subject: Unaligned access in pf_normalize_tcpopt()
X-Send-Pr-Version: www-1.0
>Number: 43541
>Category: kern
>Synopsis: Unaligned access in pf_normalize_tcpopt()
>Confidential: no
>Severity: serious
>Priority: low
>Responsible: kern-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Mon Jun 28 16:35:00 +0000 2010
>Last-Modified: Sun Jul 31 23:10:02 +0000 2016
>Originator: Gregory T. Andersen
>Release: 5.0.2
>Organization:
CradlePoint Technology
>Environment:
NetBSD 5.0.2 NetBSD 5.0.2 (PUCK) #0: Fri Jun 25 11:18:40 MDT 2010 andersen@andersen:/build/sys/arch/evbmips/compile/obj/PUCK evbmips
>Description:
Ran into a unaligned access kernel crasher on a MIPS32 architecture when certain PF rules are applied that manipulate tcp options (mss).
>How-To-Repeat:
Receive TCP packets where the MSS TCP option is unaligned on architecture that doesn't handle unaligned access with mss scrub rule in PF.
>Fix:
Potential patch:
Index: sys/dist/pf/net/pf_norm.c
===================================================================
--- sys/dist/pf/net/pf_norm.c
+++ sys/dist/pf/net/pf_norm.c (working copy)
@@ -1878,7 +1878,7 @@
pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
int off)
{
- u_int16_t *mss;
+ u_int16_t mss;
int thoff;
int opt, cnt, optlen = 0;
int rewrite = 0;
@@ -1903,11 +1903,12 @@
}
switch (opt) {
case TCPOPT_MAXSEG:
- mss = (u_int16_t *)(optp + 2);
- if ((ntohs(*mss)) > r->max_mss) {
+ mss = (optp[2] << 8) | optp[3];
+ if (mss > r->max_mss) {
th->th_sum = pf_cksum_fixup(th->th_sum,
- *mss, htons(r->max_mss), 0);
- *mss = htons(r->max_mss);
+ htons(mss), htons(r->max_mss), 0);
+ optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
+ optp[3] = (u_char)(r->max_mss) & 0xff;
rewrite = 1;
}
break;
>Audit-Trail:
From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org, Hisashi T Fujinaka <htodd@twofifty.com>
Subject: re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Tue, 29 Jun 2010 04:15:00 +1000
i wonder if this is the same problem as reported in:
port-sparc/40629: kernel panic under high network load using pf (and ipv6)
hisashi, can you try the patch in this PR for your problem?
(included below again.)
.mrg.
Index: sys/dist/pf/net/pf_norm.c
===================================================================
--- sys/dist/pf/net/pf_norm.c
+++ sys/dist/pf/net/pf_norm.c (working copy)
@@ -1878,7 +1878,7 @@
pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
int off)
{
- u_int16_t *mss;
+ u_int16_t mss;
int thoff;
int opt, cnt, optlen = 0;
int rewrite = 0;
@@ -1903,11 +1903,12 @@
}
switch (opt) {
case TCPOPT_MAXSEG:
- mss = (u_int16_t *)(optp + 2);
- if ((ntohs(*mss)) > r->max_mss) {
+ mss = (optp[2] << 8) | optp[3];
+ if (mss > r->max_mss) {
th->th_sum = pf_cksum_fixup(th->th_sum,
- *mss, htons(r->max_mss), 0);
- *mss = htons(r->max_mss);
+ htons(mss), htons(r->max_mss), 0);
+ optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
+ optp[3] = (u_char)(r->max_mss) & 0xff;
rewrite = 1;
}
break;
From: Hisashi T Fujinaka <htodd@twofifty.com>
To: matthew green <mrg@eterna.com.au>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org,
netbsd-bugs@NetBSD.org
Subject: re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Mon, 28 Jun 2010 16:01:12 -0700 (PDT)
I've switched to a sparc64 system and had a hard time causing the hang,
so I think I better ask for this to be closed. I still have the old
system, but I'm not sure how I'd trigger this.
Thanks.
On Tue, 29 Jun 2010, matthew green wrote:
>
> i wonder if this is the same problem as reported in:
>
> port-sparc/40629: kernel panic under high network load using pf (and ipv6)
>
> hisashi, can you try the patch in this PR for your problem?
> (included below again.)
>
>
> .mrg.
>
>
> Index: sys/dist/pf/net/pf_norm.c
> ===================================================================
> --- sys/dist/pf/net/pf_norm.c
> +++ sys/dist/pf/net/pf_norm.c (working copy)
> @@ -1878,7 +1878,7 @@
> pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
> int off)
> {
> - u_int16_t *mss;
> + u_int16_t mss;
> int thoff;
> int opt, cnt, optlen = 0;
> int rewrite = 0;
> @@ -1903,11 +1903,12 @@
> }
> switch (opt) {
> case TCPOPT_MAXSEG:
> - mss = (u_int16_t *)(optp + 2);
> - if ((ntohs(*mss)) > r->max_mss) {
> + mss = (optp[2] << 8) | optp[3];
> + if (mss > r->max_mss) {
> th->th_sum = pf_cksum_fixup(th->th_sum,
> - *mss, htons(r->max_mss), 0);
> - *mss = htons(r->max_mss);
> + htons(mss), htons(r->max_mss), 0);
> + optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
> + optp[3] = (u_char)(r->max_mss) & 0xff;
> rewrite = 1;
> }
> break;
>
--
Hisashi T Fujinaka - htodd@twofifty.com
BSEE(6/86) + BSChem(3/95) + BAEnglish(8/95) + MSCS(8/03) + $2.50 = latte
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Tue, 29 Jun 2010 20:12:11 +0000
On Mon, Jun 28, 2010 at 04:35:00PM +0000, gandersen@cradlepoint.com wrote:
> + optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
> + optp[3] = (u_char)(r->max_mss) & 0xff;
Do those casts serve any purpose?
otherwise, seems like a desirable patch...
--
David A. Holland
dholland@netbsd.org
From: David Laight <david@l8s.co.uk>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Wed, 30 Jun 2010 00:21:05 +0100
On Tue, Jun 29, 2010 at 08:15:05PM +0000, David Holland wrote:
> The following reply was made to PR kern/43541; it has been noted by GNATS.
>
> From: David Holland <dholland-bugs@netbsd.org>
> To: gnats-bugs@NetBSD.org
> Cc:
> Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
> Date: Tue, 29 Jun 2010 20:12:11 +0000
>
> On Mon, Jun 28, 2010 at 04:35:00PM +0000, gandersen@cradlepoint.com wrote:
> > + optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
> > + optp[3] = (u_char)(r->max_mss) & 0xff;
>
> Do those casts serve any purpose?
Do the '& 0xff' serve any purpose ?
The compiler may well generate code that 'and's the value with 0xff twice
before storing the low 8 bits away!
OTOH reversing the two lines may well save a temporary register.
Hmmm... actually you want to cache r->max_rss in a local, the compiler
is most likely required to re-read r->max_rss between the two statements.
David
--
David Laight: david@l8s.co.uk
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Tue, 29 Jun 2010 23:35:13 +0000
On Tue, Jun 29, 2010 at 11:20:05PM +0000, David Laight wrote:
>>> + optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
>>> + optp[3] = (u_char)(r->max_mss) & 0xff;
>>
>> Do those casts serve any purpose?
>
> Do the '& 0xff' serve any purpose ?
Arguably not, but they at least can't mask other bugs.
> The compiler may well generate code that 'and's the value with 0xff twice
> before storing the low 8 bits away!
Not very likely, except perhaps with gcc -O0, which is so stupid
anyway that there's no point worrying about it.
> OTOH reversing the two lines may well save a temporary register.
> Hmmm... actually you want to cache r->max_rss in a local, the compiler
> is most likely required to re-read r->max_rss between the two statements.
Why would it be? It's not IIRC marked volatile.
--
David A. Holland
dholland@netbsd.org
From: David Laight <david@l8s.co.uk>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Mon, 5 Jul 2010 21:22:46 +0100
On Tue, Jun 29, 2010 at 11:40:05PM +0000, David Holland wrote:
>
> On Tue, Jun 29, 2010 at 11:20:05PM +0000, David Laight wrote:
> >>> + optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
> >>> + optp[3] = (u_char)(r->max_mss) & 0xff;
> >>
> >> Do those casts serve any purpose?
> >
> > Do the '& 0xff' serve any purpose ?
>
> Arguably not, but they at least can't mask other bugs.
>
> > The compiler may well generate code that 'and's the value with 0xff twice
> > before storing the low 8 bits away!
>
> Not very likely, except perhaps with gcc -O0, which is so stupid
> anyway that there's no point worrying about it.
Not in my experience!
I've certainly seen compilers mask with 0xff then save the low bits.
> > OTOH reversing the two lines may well save a temporary register.
> > Hmmm... actually you want to cache r->max_rss in a local, the compiler
> > is most likely required to re-read r->max_rss between the two statements.
>
> Why would it be? It's not IIRC marked volatile.
The aliasing rules ...
David
--
David Laight: david@l8s.co.uk
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Mon, 5 Jul 2010 21:07:27 +0000
On Mon, Jul 05, 2010 at 08:20:03PM +0000, David Laight wrote:
>>> The compiler may well generate code that 'and's the value with 0xff
>>> twice before storing the low 8 bits away!
>>
>> Not very likely, except perhaps with gcc -O0, which is so stupid
>> anyway that there's no point worrying about it.
>
> Not in my experience!
> I've certainly seen compilers mask with 0xff then save the low bits.
Well, I've also *seen* compilers store temporary values to scratch
slots in the stack and immediately read them back. The question is
whether a reasonably sane and modernish compiler that's been at least
somewhat debugged is going to do anything so silly.
However, it's also moot because such micro-optimizations are not worth
spending time thinking about.
>>> OTOH reversing the two lines may well save a temporary register.
>>> Hmmm... actually you want to cache r->max_rss in a local, the compiler
>>> is most likely required to re-read r->max_rss between the two statements.
>>
>> Why would it be? It's not IIRC marked volatile.
>
> The aliasing rules ...
Hrm, perhaps, I guess proving that optp[2] and r->max_rss don't
overlap would require global analysis.
--
David A. Holland
dholland@netbsd.org
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Sun, 31 Jul 2016 23:08:15 +0000
On Mon, Jun 28, 2010 at 04:35:00PM +0000, gandersen@cradlepoint.com wrote:
> Index: sys/dist/pf/net/pf_norm.c
> ===================================================================
> --- sys/dist/pf/net/pf_norm.c
> +++ sys/dist/pf/net/pf_norm.c (working copy)
> @@ -1878,7 +1878,7 @@
> pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
> int off)
> {
> - u_int16_t *mss;
> + u_int16_t mss;
> int thoff;
> int opt, cnt, optlen = 0;
> int rewrite = 0;
> @@ -1903,11 +1903,12 @@
> }
> switch (opt) {
> case TCPOPT_MAXSEG:
> - mss = (u_int16_t *)(optp + 2);
> - if ((ntohs(*mss)) > r->max_mss) {
> + mss = (optp[2] << 8) | optp[3];
> + if (mss > r->max_mss) {
> th->th_sum = pf_cksum_fixup(th->th_sum,
> - *mss, htons(r->max_mss), 0);
> - *mss = htons(r->max_mss);
> + htons(mss), htons(r->max_mss), 0);
> + optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
> + optp[3] = (u_char)(r->max_mss) & 0xff;
> rewrite = 1;
> }
> break;
>
Is there any reason this patch hasn't been committed?
--
David A. Holland
dholland@netbsd.org
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.