Date: Fri, 10 Jan 2014 13:56:32 +0200 From: Guy Yur <guyyur@gmail.com> To: freebsd-net@freebsd.org, freebsd-arm@freebsd.org Subject: Re: 10.0-RC1, armv6: "pfctl -s state" crashes on BeagleBone Black due to unaligned access Message-ID: <CAC67Hz_-CAknHq07o2tVw5xGHkKZGRLhAKdaYtJZHwdAPriK7g@mail.gmail.com> In-Reply-To: <20140109233858.GL46596@funkthat.com> References: <CAC67Hz_QXcHHSFOLLgUGqLWRQpzhRRv_b%2BWGMMQsfk-VQp74RA@mail.gmail.com> <20140109104223.GS71033@FreeBSD.org> <CAC67Hz-Rz557COtyE1AurduZrstOqaMaA_H9VzBypsaHfSc=cg@mail.gmail.com> <20140109222610.GJ46596@funkthat.com> <CAC67Hz--9ur8wLbqkB=aw8fK9MXjokZi9qULVa-ox_uubUz0vQ@mail.gmail.com> <20140109233858.GL46596@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jan 10, 2014 at 1:38 AM, John-Mark Gurney <jmg@funkthat.com> 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 <jmg@funkthat.com> 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 <glebius@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAC67Hz_-CAknHq07o2tVw5xGHkKZGRLhAKdaYtJZHwdAPriK7g>