Date: Fri, 25 May 2012 19:57:01 +0000 (UTC) From: Marius Strobl <marius@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org Subject: svn commit: r236022 - stable/8/sys/netinet6 Message-ID: <201205251957.q4PJv1T8030213@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: marius Date: Fri May 25 19:57:01 2012 New Revision: 236022 URL: http://svn.freebsd.org/changeset/base/236022 Log: MFC: r235681 Rewrite nd6_sysctl_{d,p}rlist() to avoid misaligned accesses to char arrays casted to structs by getting rid of these buffers entirely. In r169832, it was tried to paper over this issue by 32-bit aligning the buffers. Depending on compiler optimizations that still was insufficient for 64-bit architectures with strong alignment requirements though. While at it, add comments regarding the total lack of locking in this area. Tested by: bz Reviewed by: bz (slightly earlier version), yongari (earlier version) Modified: stable/8/sys/netinet6/nd6.c Directory Properties: stable/8/sys/ (props changed) stable/8/sys/amd64/include/xen/ (props changed) stable/8/sys/boot/ (props changed) stable/8/sys/cddl/contrib/opensolaris/ (props changed) stable/8/sys/contrib/dev/acpica/ (props changed) stable/8/sys/contrib/pf/ (props changed) stable/8/sys/dev/e1000/ (props changed) Modified: stable/8/sys/netinet6/nd6.c ============================================================================== --- stable/8/sys/netinet6/nd6.c Fri May 25 19:56:40 2012 (r236021) +++ stable/8/sys/netinet6/nd6.c Fri May 25 19:57:01 2012 (r236022) @@ -2146,128 +2146,101 @@ SYSCTL_VNET_INT(_net_inet6_icmp6, ICMPV6 static int nd6_sysctl_drlist(SYSCTL_HANDLER_ARGS) { - int error; - char buf[1024] __aligned(4); - struct in6_defrouter *d, *de; + struct in6_defrouter d; struct nd_defrouter *dr; + int error; if (req->newptr) - return EPERM; - error = 0; + return (EPERM); + bzero(&d, sizeof(d)); + d.rtaddr.sin6_family = AF_INET6; + d.rtaddr.sin6_len = sizeof(d.rtaddr); + + /* + * XXX locking + */ TAILQ_FOREACH(dr, &V_nd_defrouter, dr_entry) { - d = (struct in6_defrouter *)buf; - de = (struct in6_defrouter *)(buf + sizeof(buf)); - - if (d + 1 <= de) { - bzero(d, sizeof(*d)); - d->rtaddr.sin6_family = AF_INET6; - d->rtaddr.sin6_len = sizeof(d->rtaddr); - d->rtaddr.sin6_addr = dr->rtaddr; - error = sa6_recoverscope(&d->rtaddr); - if (error != 0) - return (error); - d->flags = dr->flags; - d->rtlifetime = dr->rtlifetime; - d->expire = dr->expire; - d->if_index = dr->ifp->if_index; - } else - panic("buffer too short"); - - error = SYSCTL_OUT(req, buf, sizeof(*d)); - if (error) - break; + d.rtaddr.sin6_addr = dr->rtaddr; + error = sa6_recoverscope(&d.rtaddr); + if (error != 0) + return (error); + d.flags = dr->flags; + d.rtlifetime = dr->rtlifetime; + d.expire = dr->expire; + d.if_index = dr->ifp->if_index; + error = SYSCTL_OUT(req, &d, sizeof(d)); + if (error != 0) + return (error); } - - return (error); + return (0); } static int nd6_sysctl_prlist(SYSCTL_HANDLER_ARGS) { - int error; - char buf[1024] __aligned(4); - struct in6_prefix *p, *pe; + struct in6_prefix p; + struct sockaddr_in6 s6; struct nd_prefix *pr; + struct nd_pfxrouter *pfr; + time_t maxexpire; + int error; char ip6buf[INET6_ADDRSTRLEN]; if (req->newptr) - return EPERM; - error = 0; + return (EPERM); + bzero(&p, sizeof(p)); + p.origin = PR_ORIG_RA; + bzero(&s6, sizeof(s6)); + s6.sin6_family = AF_INET6; + s6.sin6_len = sizeof(s6); + + /* + * XXX locking + */ LIST_FOREACH(pr, &V_nd_prefix, ndpr_entry) { - u_short advrtrs; - size_t advance; - struct sockaddr_in6 *sin6, *s6; - struct nd_pfxrouter *pfr; - - p = (struct in6_prefix *)buf; - pe = (struct in6_prefix *)(buf + sizeof(buf)); - - if (p + 1 <= pe) { - bzero(p, sizeof(*p)); - sin6 = (struct sockaddr_in6 *)(p + 1); - - p->prefix = pr->ndpr_prefix; - if (sa6_recoverscope(&p->prefix)) { + p.prefix = pr->ndpr_prefix; + if (sa6_recoverscope(&p.prefix)) { + log(LOG_ERR, "scope error in prefix list (%s)\n", + ip6_sprintf(ip6buf, &p.prefix.sin6_addr)); + /* XXX: press on... */ + } + p.raflags = pr->ndpr_raf; + p.prefixlen = pr->ndpr_plen; + p.vltime = pr->ndpr_vltime; + p.pltime = pr->ndpr_pltime; + p.if_index = pr->ndpr_ifp->if_index; + if (pr->ndpr_vltime == ND6_INFINITE_LIFETIME) + p.expire = 0; + else { + /* XXX: we assume time_t is signed. */ + maxexpire = (-1) & + ~((time_t)1 << ((sizeof(maxexpire) * 8) - 1)); + if (pr->ndpr_vltime < maxexpire - pr->ndpr_lastupdate) + p.expire = pr->ndpr_lastupdate + + pr->ndpr_vltime; + else + p.expire = maxexpire; + } + p.refcnt = pr->ndpr_refcnt; + p.flags = pr->ndpr_stateflags; + p.advrtrs = 0; + LIST_FOREACH(pfr, &pr->ndpr_advrtrs, pfr_entry) + p.advrtrs++; + error = SYSCTL_OUT(req, &p, sizeof(p)); + if (error != 0) + return (error); + LIST_FOREACH(pfr, &pr->ndpr_advrtrs, pfr_entry) { + s6.sin6_addr = pfr->router->rtaddr; + if (sa6_recoverscope(&s6)) log(LOG_ERR, "scope error in prefix list (%s)\n", - ip6_sprintf(ip6buf, &p->prefix.sin6_addr)); - /* XXX: press on... */ - } - p->raflags = pr->ndpr_raf; - p->prefixlen = pr->ndpr_plen; - p->vltime = pr->ndpr_vltime; - p->pltime = pr->ndpr_pltime; - p->if_index = pr->ndpr_ifp->if_index; - if (pr->ndpr_vltime == ND6_INFINITE_LIFETIME) - p->expire = 0; - else { - time_t maxexpire; - - /* XXX: we assume time_t is signed. */ - maxexpire = (-1) & - ~((time_t)1 << - ((sizeof(maxexpire) * 8) - 1)); - if (pr->ndpr_vltime < - maxexpire - pr->ndpr_lastupdate) { - p->expire = pr->ndpr_lastupdate + - pr->ndpr_vltime; - } else - p->expire = maxexpire; - } - p->refcnt = pr->ndpr_refcnt; - p->flags = pr->ndpr_stateflags; - p->origin = PR_ORIG_RA; - advrtrs = 0; - LIST_FOREACH(pfr, &pr->ndpr_advrtrs, pfr_entry) { - if ((void *)&sin6[advrtrs + 1] > (void *)pe) { - advrtrs++; - continue; - } - s6 = &sin6[advrtrs]; - bzero(s6, sizeof(*s6)); - s6->sin6_family = AF_INET6; - s6->sin6_len = sizeof(*sin6); - s6->sin6_addr = pfr->router->rtaddr; - if (sa6_recoverscope(s6)) { - log(LOG_ERR, - "scope error in " - "prefix list (%s)\n", - ip6_sprintf(ip6buf, - &pfr->router->rtaddr)); - } - advrtrs++; - } - p->advrtrs = advrtrs; - } else - panic("buffer too short"); - - advance = sizeof(*p) + sizeof(*sin6) * advrtrs; - error = SYSCTL_OUT(req, buf, advance); - if (error) - break; + ip6_sprintf(ip6buf, &pfr->router->rtaddr)); + error = SYSCTL_OUT(req, &s6, sizeof(s6)); + if (error != 0) + return (error); + } } - - return (error); + return (0); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201205251957.q4PJv1T8030213>