Date: Wed, 10 Feb 2010 10:48:09 +0100 From: =?UTF-8?Q?Marius_N=C3=BCnnerich?= <marius@nuenneri.ch> To: "M. Warner Losh" <imp@bsdimp.com> Cc: net@freebsd.org Subject: Re: struct sockaddr * and alignment Message-ID: <b649e5e1002100148r759f3aacr3d5fcdfb5efd9001@mail.gmail.com> In-Reply-To: <20100209.103411.506052712871889475.imp@bsdimp.com> References: <20100209.103411.506052712871889475.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 9, 2010 at 18:34, M. Warner Losh <imp@bsdimp.com> wrote:
> Greetings,
>
> I've found a few inconsistencies in the sockaddr stuff in the tree.
> I'm not sure where to go to get a definitive answer, so I thought I'd
> start here.
>
> I got here looking at the recent wake breakage on mips. =C2=A0It turns ou=
t
> that the warning was:
>
> src/usr.sbin/wake/wake.c: In function 'find_ether':
> src/usr.sbin/wake/wake.c:123: warning: cast increases required alignment =
of target type
>
> which comes from
> =C2=A0 =C2=A0 =C2=A0 =C2=A0sdl =3D (struct sockaddr_dl *)ifa->ifa_addr;
>
> The problem is that on MIPS struct sockaddr * is byte aligned and
> sockaddr_dl * is word aligned, so the compiler is rightly telling us
> that there might be a problem here.
>
> However, further digging shows that there will never be a problem
> here with alignment. =C2=A0struct sockaddr_storage has a int64 in it to
> force it to be __aligned(8). =C2=A0So I thought to myself "why don't I ju=
st
> add __aligned(8) to the struct sockaddr definition?" =C2=A0After all, the
> kernel goes to great lengths to return data so aligned, and user code
> also keeps things aligned.
>
> Sure enough, that fixes this warning. =C2=A0Yea. =C2=A0But, sadly, it cau=
ses
> other problems. =C2=A0If you look at sbin/atm/atmconfig/natm.c you'll see
> code like:
>
> static void
> store_route(struct rt_msghdr *rtm)
> {
> ...
> =C2=A0 =C2=A0 =C2=A0 =C2=A0char *cp
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct sockaddr *sa;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0...
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0cp =3D (char *)(rtm + 1);
> ...
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0sa =3D (struct sockaddr *)cp;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0cp +=3D roundup(sa->sa_len, sizeof(long));
> ...
>
> which breaks because we're now casting from an __aligned(1) char * to
> an __aligned(8) sockaddr *.
>
> And it is only rounding the size of the structure to long, rather than
> int64 like sockaddr_storage suggests is the proper alignment. =C2=A0But I
> haven't looked in the kernel to see if there's an issue there with
> routing sockets or not.
>
> The other extreme is to put __aligned(1) on all the sockaddr_foo
> structures. =C2=A0This would solve the compiler warning, but would have a
> negative effect on performance in accessing these elements (because
> the compiler would have to generate calls to bcopy or equivalent to
> access the scalar members that are larger than a byte). =C2=A0 This cure
> would be worse than the disease.
>
> So the question here is "What is the right solution here?" =C2=A0It has m=
e
> stumped. =C2=A0So I dropped WARNS level down from 6 to 3 for wake.c.
Hi Warner,
I got into the same kind of trouble when I tried to raise the WARNS
level above 3 for inetd and others. I guess everything which uses some
sockaddr casting or (in the case of inetd) some of these macros:
http://fxr.googlebit.com/source/sys/netinet6/in6.h?v=3D8-CURRENT#L233
It's a pity that only this keeps some programs from going to a WARNS level =
of 6.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b649e5e1002100148r759f3aacr3d5fcdfb5efd9001>