From owner-freebsd-net@FreeBSD.ORG Fri Jan 10 11:56:34 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E8729E81; Fri, 10 Jan 2014 11:56:33 +0000 (UTC) Received: from mail-oa0-x22e.google.com (mail-oa0-x22e.google.com [IPv6:2607:f8b0:4003:c02::22e]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 9DD74152D; Fri, 10 Jan 2014 11:56:33 +0000 (UTC) Received: by mail-oa0-f46.google.com with SMTP id l6so4849351oag.19 for ; Fri, 10 Jan 2014 03:56:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=sLUlKQy0MoIiYGhsbK3mHa3pgCcUvH7CqSp3nhZhKa0=; b=dvGRpu/ULv0x1ynxhtYNNwFCC8V/lsirExSkHf3mm8UerHprJPcM+N9KRtYRiUUR+D xTOTcMIqREmUaasz769unCrgKXDBCpZ/sDQesASfYhZSnUbSYDRyvACb5GTPyrwfqEk2 KyuzQNybPlKQrleEhclLPa6ai+ko07qRXczMinklJ1mZu1kk5o+Oil9QuIxAES/iZKF0 WDpoJ+GMsH/OAT9qFMVAdyeaJcOZNuOsaKObufUIsgN2NYZQq3Qt8n0TwCx4BDCs94g4 MrdjDNImfdZXVAMATXPZlwMel7JQGbdfBTw6b/pC93LosJLticcwreQ/Eo8/60cLkfXl nvkw== MIME-Version: 1.0 X-Received: by 10.60.62.199 with SMTP id a7mr1778866oes.64.1389354992911; Fri, 10 Jan 2014 03:56:32 -0800 (PST) Received: by 10.76.20.82 with HTTP; Fri, 10 Jan 2014 03:56:32 -0800 (PST) In-Reply-To: <20140109233858.GL46596@funkthat.com> References: <20140109104223.GS71033@FreeBSD.org> <20140109222610.GJ46596@funkthat.com> <20140109233858.GL46596@funkthat.com> Date: Fri, 10 Jan 2014 13:56:32 +0200 Message-ID: Subject: Re: 10.0-RC1, armv6: "pfctl -s state" crashes on BeagleBone Black due to unaligned access From: Guy Yur To: freebsd-net@freebsd.org, freebsd-arm@freebsd.org Content-Type: text/plain; charset=UTF-8 X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Jan 2014 11:56:34 -0000 On Fri, Jan 10, 2014 at 1:38 AM, John-Mark Gurney wrote: > Guy Yur wrote this message on Fri, Jan 10, 2014 at 01:04 +0200: >> On Fri, Jan 10, 2014 at 12:26 AM, John-Mark Gurney wrote: >> > Guy Yur wrote this message on Fri, Jan 10, 2014 at 00:17 +0200: >> >> On Thu, Jan 9, 2014 at 12:42 PM, Gleb Smirnoff wrote: >> >> > Guy, >> >> > >> >> > On Sat, Jan 04, 2014 at 03:06:02PM +0200, Guy Yur wrote: >> >> > G> I am running 10.0-RC1 arm.armv6 on the BeagleBone Black. >> >> > G> The "pfctl -s state" command is crashing when trying to print the >> >> > G> second entry. >> >> >> >> > Ok, that makes sense... so, either we mark struct pf_addr as __packed, >> > or we do some nasty stuff, like the following in print_host: >> > struct { >> > struct pf_addr a >> > } *uaddr __packed; >> > >> > uaddr = addr; >> > aw.v.a.addr = uaddr->a; >> > >> > it's not pretty, but I believe it would work... >> >> For performance reasons, I don't think pf_addr should be marked as __packed. >> >> I attached the changes I am now using in print_state() since there is >> no need to copy >> the full pfsync_state, only pf_addr. >> I converted sk and nk from pointers to structs on the stack and using >> struct copy. >> pf_addr is 16 bytes. > > Did you look at using the above trick? > > Since we are iterating over a list, that'll be a lot of copies, plus, > I'm not sure that your fix will be guaranteed to work for ever.. since > there isn't a requirement that the copy happens w/ bcopy/memcpy or some > other copy routine that assumes things might not be aligned... > Right. The correct fix would be to have a separate struct for the ioctl that can be aligned as Gleb suggested. I will try to prepare and test such changes. If new ioctls are added, the KBI can also be preserved. pfsync_state_export is used by if_pfsync.c and pf_ioctl.c so there will be duplicated code even if reusing the old ioctls with the new struct. > Specificly these: > - sk = &s->key[PF_SK_STACK]; > - nk = &s->key[PF_SK_WIRE]; > + sk = s->key[PF_SK_STACK]; > + nk = s->key[PF_SK_WIRE]; > > since s->key is already assumed to be aligned, a future compiler could > be smart enough to say, I'm not going to use the stack.. That > would/could happen if print_host's addr arg grew a const which it > could... > I thought that because s itself is __packed and key is an array inside s the __packed will apply to it too, since the disassembly showed clang chose to do a memcpy, I don't know if that is true or not. An explicit bcopy is safer. I see that the function already does a bcopy of the u_int64_t id field so it has some assumption that the structure might not be aligned. If a new always aligned structure is used for the ioctl, the bcopy of id can also be avoided. > Also, I just realized that some of the lines modify sk (setting port), > but you don't write those modifications back to s->key[PF_SK_STACK]... > The print function doesn't need to modify s->key, the port changes are only for passing to print_host. > -- > John-Mark Gurney Voice: +1 415 225 5579 > > "All that I will do, has been done, All that I have, has not." Regards, Guy