From owner-freebsd-current@FreeBSD.ORG Mon Jun 2 22:24:21 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 75A9F37B401; Mon, 2 Jun 2003 22:24:21 -0700 (PDT) Received: from magic.adaptec.com (magic-mail.adaptec.com [208.236.45.100]) by mx1.FreeBSD.org (Postfix) with ESMTP id B348D43F75; Mon, 2 Jun 2003 22:24:20 -0700 (PDT) (envelope-from samsco@mho.com) Received: from redfish.adaptec.com (redfish.adaptec.com [162.62.50.11]) by magic.adaptec.com (8.11.6/8.11.6) with ESMTP id h535JJZ25413; Mon, 2 Jun 2003 22:19:19 -0700 Received: from mho.com (hollin.btc.adaptec.com [10.100.253.56]) by redfish.adaptec.com (8.8.8p2+Sun/8.8.8) with ESMTP id WAA13625; Mon, 2 Jun 2003 22:24:17 -0700 (PDT) Message-ID: <3EDC30F3.7040305@mho.com> Date: Mon, 02 Jun 2003 23:24:03 -0600 From: Scott Long User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.3) Gecko/20030425 X-Accept-Language: en-us, en MIME-Version: 1.0 To: ticso@cicely.de References: <3ED94166.7070300@btc.adaptec.com> <20030531173958.C91048@xorpc.icir.org> <20030601013256.GH503@cicely12.cicely.de> <20030601022633.A4287@xorpc.icir.org> <20030601130008.GA527@cicely12.cicely.de> <20030602132847.GH527@cicely12.cicely.de> In-Reply-To: <20030602132847.GH527@cicely12.cicely.de> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit cc: Luigi Rizzo cc: Robert Watson cc: current@freebsd.org Subject: Re: 5.1-RELEASE TODO X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: scottl@freebsd.org List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Jun 2003 05:24:21 -0000 Where do we stand on this now? Is this ready for committing? Is it completely solved? Scott Bernd Walter wrote: > On Sun, Jun 01, 2003 at 03:00:09PM +0200, Bernd Walter wrote: > >>On Sun, Jun 01, 2003 at 02:26:34AM -0700, Luigi Rizzo wrote: >> >>>On Sun, Jun 01, 2003 at 03:32:56AM +0200, Bernd Walter wrote: >>>... >>> >>>>:) >>>>And I hoped a programmer who knows the source could find out and fix >>>>very quickly. >>> >>>sorry, i missed the offending line number in your previous email. >>> >>>I think i missed a & in all the first arguments to bcopy in >>>the src/sbin/ipfw2.c changes :( >>> >>>this happens at lines 818, 1224, 1461 and 1701. Fortunately >>>the kernel part seems correct. >>> >>>In detail, the fix should be the following: >>> >>>818: >>>- bcopy(rule->next_rule, &set_disable, sizeof(set_disable)); >>>+ bcopy(&rule->next_rule, &set_disable, sizeof(set_disable)); >>> >>>1224: >>>- bcopy(d->rule, &rulenum, sizeof(rulenum)); >>>+ bcopy(&d->rule, &rulenum, sizeof(rulenum)); >>> >>>1461: >>>- bcopy(((struct ip_fw *)data)->next_rule, >>>+ bcopy(&((struct ip_fw *)data)->next_rule, >>> >>>1701: >>>- bcopy(d->rule, &rulenum, sizeof(rulenum)); >>>+ bcopy(&d->rule, &rulenum, sizeof(rulenum)); >> >>Look way bettter now :) >>I wasn't able to crash the kernel with missaligned access any more, but >>the userland tool still does in some situations: >>[59]cicely12# ipfw show >>pid 2121 (ipfw): unaligned access: va=0x1200ac09c pc=0x120003bb4 ra=0x120003bfc op=ldq >>pid 2121 (ipfw): unaligned access: va=0x1200ac0a4 pc=0x120003bdc ra=0x120003bc8 op=ldq >>00100 5237 824333 allow tcp from any to any dst-port 1-65535,1-65535 >>00200 0 0 allow tcp from any to any dst-port 1-65535,1-65535,1-65535 >>pid 2121 (ipfw): unaligned access: va=0x1200ac09c pc=0x120002260 ra=0x1200015ec op=ldq >>pid 2121 (ipfw): unaligned access: va=0x1200ac0a4 pc=0x120002264 ra=0x1200015ec op=ldq >>65535 5836817 1002036976 allow ip from any to any > > > I'm currently using the attached diff to ipfw2.c + your other changes. > It seems to work now. > I hope that I catched all missalignemts that were missing. > > Thanks for the work on this. > I'm very happy to see this running on alpha. > > > > ------------------------------------------------------------------------ > > Index: ipfw2.c > =================================================================== > RCS file: /home/ncvs/src/sbin/ipfw/ipfw2.c,v > retrieving revision 1.23 > diff -u -r1.23 ipfw2.c > --- ipfw2.c 15 Mar 2003 01:12:59 -0000 1.23 > +++ ipfw2.c 2 Jun 2003 13:22:30 -0000 > @@ -348,6 +348,14 @@ > { NULL, 0 } > }; > > +static __inline u_int64_t > +align_uint64(u_int64_t *pll) { > + u_int64_t ret; > + > + bcopy (pll, &ret, sizeof(ret)); > + return ret; > +}; > + > /** > * match_token takes a table and a string, returns the value associated > * with the string (0 meaning an error in most cases) > @@ -813,8 +821,9 @@ > int flags = 0; /* prerequisites */ > ipfw_insn_log *logptr = NULL; /* set if we find an O_LOG */ > int or_block = 0; /* we are in an or block */ > + u_int32_t set_disable; > > - u_int32_t set_disable = rule->set_disable; > + bcopy(&rule->next_rule, &set_disable, sizeof(set_disable)); > > if (set_disable & (1 << rule->set)) { /* disabled */ > if (!show_sets) > @@ -825,8 +834,8 @@ > printf("%05u ", rule->rulenum); > > if (do_acct) > - printf("%*llu %*llu ", pcwidth, rule->pcnt, bcwidth, > - rule->bcnt); > + printf("%*llu %*llu ", pcwidth, align_uint64(&rule->pcnt), > + bcwidth, align_uint64(&rule->bcnt)); > > if (do_time) { > char timestr[30]; > @@ -1213,13 +1222,16 @@ > { > struct protoent *pe; > struct in_addr a; > + uint16_t rulenum; > > if (!do_expired) { > if (!d->expire && !(d->dyn_type == O_LIMIT_PARENT)) > return; > } > > - printf("%05d %*llu %*llu (%ds)", d->rulenum, pcwidth, d->pcnt, bcwidth, > + bcopy(&d->rule, &rulenum, sizeof(rulenum)); > + > + printf("%05d %*llu %*llu (%ds)", rulenum, pcwidth, d->pcnt, bcwidth, > d->bcnt, d->expire); > switch (d->dyn_type) { > case O_LIMIT_PARENT: > @@ -1454,7 +1466,9 @@ > err(EX_OSERR, "malloc"); > if (getsockopt(s, IPPROTO_IP, IP_FW_GET, data, &nbytes) < 0) > err(EX_OSERR, "getsockopt(IP_FW_GET)"); > - set_disable = ((struct ip_fw *)data)->set_disable; > + bcopy(&((struct ip_fw *)data)->next_rule, > + &set_disable, sizeof(set_disable)); > + > > for (i = 0, msg = "disable" ; i < 31; i++) > if ( (set_disable & (1< @@ -1620,23 +1634,27 @@ > for (n = 0, r = data; n < nstat; > n++, r = (void *)r + RULESIZE(r)) { > /* packet counter */ > - width = snprintf(NULL, 0, "%llu", r->pcnt); > + width = snprintf(NULL, 0, "%llu", > + align_uint64(&r->pcnt)); > if (width > pcwidth) > pcwidth = width; > > /* byte counter */ > - width = snprintf(NULL, 0, "%llu", r->bcnt); > + width = snprintf(NULL, 0, "%llu", > + align_uint64(&r->bcnt)); > if (width > bcwidth) > bcwidth = width; > } > } > if (do_dynamic && ndyn) { > for (n = 0, d = dynrules; n < ndyn; n++, d++) { > - width = snprintf(NULL, 0, "%llu", d->pcnt); > + width = snprintf(NULL, 0, "%llu", > + align_uint64(&d->pcnt)); > if (width > pcwidth) > pcwidth = width; > > - width = snprintf(NULL, 0, "%llu", d->bcnt); > + width = snprintf(NULL, 0, "%llu", > + align_uint64(&d->bcnt)); > if (width > bcwidth) > bcwidth = width; > } > @@ -1690,9 +1708,12 @@ > /* already warned */ > continue; > for (n = 0, d = dynrules; n < ndyn; n++, d++) { > - if (d->rulenum > rnum) > + uint16_t rulenum; > + > + bcopy(&d->rule, &rulenum, sizeof(rulenum)); > + if (rulenum > rnum) > break; > - if (d->rulenum == rnum) > + if (rulenum == rnum) > show_dyn_ipfw(d, pcwidth, bcwidth); > } > }