Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 25 May 2012 19:56:40 +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-9@freebsd.org
Subject:   svn commit: r236021 - stable/9/sys/netinet6
Message-ID:  <201205251956.q4PJue87030164@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: marius
Date: Fri May 25 19:56:40 2012
New Revision: 236021
URL: http://svn.freebsd.org/changeset/base/236021

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/9/sys/netinet6/nd6.c
Directory Properties:
  stable/9/sys/   (props changed)
  stable/9/sys/amd64/include/xen/   (props changed)
  stable/9/sys/boot/   (props changed)
  stable/9/sys/boot/i386/efi/   (props changed)
  stable/9/sys/boot/ia64/efi/   (props changed)
  stable/9/sys/boot/ia64/ski/   (props changed)
  stable/9/sys/boot/powerpc/boot1.chrp/   (props changed)
  stable/9/sys/boot/powerpc/ofw/   (props changed)
  stable/9/sys/cddl/contrib/opensolaris/   (props changed)
  stable/9/sys/conf/   (props changed)
  stable/9/sys/contrib/dev/acpica/   (props changed)
  stable/9/sys/contrib/octeon-sdk/   (props changed)
  stable/9/sys/contrib/pf/   (props changed)
  stable/9/sys/contrib/x86emu/   (props changed)
  stable/9/sys/dev/   (props changed)
  stable/9/sys/dev/e1000/   (props changed)
  stable/9/sys/dev/ixgbe/   (props changed)
  stable/9/sys/fs/   (props changed)
  stable/9/sys/fs/ntfs/   (props changed)
  stable/9/sys/modules/   (props changed)

Modified: stable/9/sys/netinet6/nd6.c
==============================================================================
--- stable/9/sys/netinet6/nd6.c	Fri May 25 19:45:01 2012	(r236020)
+++ stable/9/sys/netinet6/nd6.c	Fri May 25 19:56:40 2012	(r236021)
@@ -2275,128 +2275,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?201205251956.q4PJue87030164>