On Wed, May 02, 2018 at 04:25:20PM +0200, Hrvoje Popovski wrote:
> On 2.5.2018. 12:16, Theo Buehler wrote:
> > Here's a further refactoring of in6_ioctl() that splits out the
> > SIOCGIF*_IN6 cases into the new function in6_ioctl_get(), where we only
> > need a read lock, similar to ifioctl() and the pending patch for
> > in_ioctl(). We get rid of one of the four switches in the function body
> > and error out early on on unknown ioctls before grabbing a lock instead
> > of doing nothing until the end of the function.
> >
> > After grabbing the lock in the body of in6_ioctl(), we now only deal
> > with SIOCAIFADDR_IN6 and SIOCDIFADDR_IN6. This will need more cleanup
> > and streamlining in a later step.
> >
> > I didn't really know what to do with the big comment. I left it
> > essentially where it was. Suggestions welcome.
>
> Hi,
>
> i'm testing this diff on top on -current tree fetched hour ago. i'm
> forwarding ip6 traffic over vlan on trunk and doing ip6 client server
> with iperf3 while destroying and recreating pseudo interfaces
>
> for now everything seems stable
>
Thanks, that's good to know. However, I overlooked a shadowing issue.
There was a local re-declaration of error, making the function succeed
more often than it should. This additional piece is needed:
switch(cmd) {
case SIOCAIFADDR_IN6:
{
- int plen, error = 0, newifaddr = 0;
+ int plen, newifaddr = 0;
This is the only change to the previous submission.
Index: sys/netinet6/in6.c
===================================================================
RCS file: /var/cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.223
diff -u -p -r1.223 in6.c
--- sys/netinet6/in6.c 2 May 2018 07:19:45 -0000 1.223
+++ sys/netinet6/in6.c 2 May 2018 23:10:05 -0000
@@ -116,6 +116,7 @@ const struct in6_addr in6mask96 = IN6MAS
const struct in6_addr in6mask128 = IN6MASK128;
int in6_ioctl(u_long, caddr_t, struct ifnet *, int);
+int in6_ioctl_get(u_long, caddr_t, struct ifnet *);
int in6_ifinit(struct ifnet *, struct in6_ifaddr *, int);
void in6_unlink_ifa(struct in6_ifaddr *, struct ifnet *);
@@ -218,29 +219,15 @@ in6_ioctl(u_long cmd, caddr_t data, stru
case SIOCGIFINFO_IN6:
case SIOCGNBRINFO_IN6:
return (nd6_ioctl(cmd, data, ifp));
- }
-
- /*
- * Find address for this interface, if it exists.
- *
- * In netinet code, we have checked ifra_addr in SIOCSIF*ADDR operation
- * only, and used the first interface address as the target of other
- * operations (without checking ifra_addr). This was because netinet
- * code/API assumed at most 1 interface address per interface.
- * Since IPv6 allows a node to assign multiple addresses
- * on a single interface, we almost always look and check the
- * presence of ifra_addr, and reject invalid ones here.
- * It also decreases duplicated code among SIOC*_IN6 operations.
- */
- switch (cmd) {
- case SIOCAIFADDR_IN6:
- sa6 = &ifra->ifra_addr;
- break;
case SIOCGIFDSTADDR_IN6:
case SIOCGIFNETMASK_IN6:
- case SIOCDIFADDR_IN6:
case SIOCGIFAFLAG_IN6:
case SIOCGIFALIFETIME_IN6:
+ return (in6_ioctl_get(cmd, data, ifp));
+ case SIOCAIFADDR_IN6:
+ sa6 = &ifra->ifra_addr;
+ break;
+ case SIOCDIFADDR_IN6:
sa6 = &ifr->ifr_addr;
break;
case SIOCSIFADDR:
@@ -253,10 +240,22 @@ in6_ioctl(u_long cmd, caddr_t data, stru
*/
return (EINVAL);
default:
- sa6 = NULL;
- break;
+ return (EOPNOTSUPP);
}
+ /*
+ * Find address for this interface, if it exists.
+ *
+ * In netinet code, we have checked ifra_addr in SIOCSIF*ADDR operation
+ * only, and used the first interface address as the target of other
+ * operations (without checking ifra_addr). This was because netinet
+ * code/API assumed at most 1 interface address per interface.
+ * Since IPv6 allows a node to assign multiple addresses
+ * on a single interface, we almost always look and check the
+ * presence of ifra_addr, and reject invalid ones here.
+ * It also decreases duplicated code among SIOC*_IN6 operations.
+ */
+
NET_LOCK();
if (sa6 && sa6->sin6_family == AF_INET6) {
@@ -312,93 +311,12 @@ in6_ioctl(u_long cmd, caddr_t data, stru
goto err;
}
break;
-
- case SIOCGIFAFLAG_IN6:
- case SIOCGIFNETMASK_IN6:
- case SIOCGIFDSTADDR_IN6:
- case SIOCGIFALIFETIME_IN6:
- /* must think again about its semantics */
- if (ia6 == NULL) {
- error = EADDRNOTAVAIL;
- goto err;
- }
- break;
}
switch (cmd) {
- case SIOCGIFDSTADDR_IN6:
- if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
- error = EINVAL;
- break;
- }
- /*
- * XXX: should we check if ifa_dstaddr is NULL and return
- * an error?
- */
- ifr->ifr_dstaddr = ia6->ia_dstaddr;
- break;
-
- case SIOCGIFNETMASK_IN6:
- ifr->ifr_addr = ia6->ia_prefixmask;
- break;
-
- case SIOCGIFAFLAG_IN6:
- ifr->ifr_ifru.ifru_flags6 = ia6->ia6_flags;
- break;
-
- case SIOCGIFALIFETIME_IN6:
- ifr->ifr_ifru.ifru_lifetime = ia6->ia6_lifetime;
- if (ia6->ia6_lifetime.ia6t_vltime != ND6_INFINITE_LIFETIME) {
- time_t expire, maxexpire;
- struct in6_addrlifetime *retlt =
- &ifr->ifr_ifru.ifru_lifetime;
-
- /*
- * XXX: adjust expiration time assuming time_t is
- * signed.
- */
- maxexpire =
- (time_t)~(1ULL << ((sizeof(maxexpire) * 8) - 1));
- if (ia6->ia6_lifetime.ia6t_vltime <
- maxexpire - ia6->ia6_updatetime) {
- expire = ia6->ia6_updatetime +
- ia6->ia6_lifetime.ia6t_vltime;
- if (expire != 0) {
- expire -= time_uptime;
- expire += time_second;
- }
- retlt->ia6t_expire = expire;
- } else
- retlt->ia6t_expire = maxexpire;
- }
- if (ia6->ia6_lifetime.ia6t_pltime != ND6_INFINITE_LIFETIME) {
- time_t expire, maxexpire;
- struct in6_addrlifetime *retlt =
- &ifr->ifr_ifru.ifru_lifetime;
-
- /*
- * XXX: adjust expiration time assuming time_t is
- * signed.
- */
- maxexpire =
- (time_t)~(1ULL << ((sizeof(maxexpire) * 8) - 1));
- if (ia6->ia6_lifetime.ia6t_pltime <
- maxexpire - ia6->ia6_updatetime) {
- expire = ia6->ia6_updatetime +
- ia6->ia6_lifetime.ia6t_pltime;
- if (expire != 0) {
- expire -= time_uptime;
- expire += time_second;
- }
- retlt->ia6t_preferred = expire;
- } else
- retlt->ia6t_preferred = maxexpire;
- }
- break;
-
case SIOCAIFADDR_IN6:
{
- int plen, error = 0, newifaddr = 0;
+ int plen, newifaddr = 0;
/* reject read-only flags */
if ((ifra->ifra_flags & IN6_IFF_DUPLICATED) != 0 ||
@@ -469,14 +387,131 @@ in6_ioctl(u_long cmd, caddr_t data, stru
in6_purgeaddr(&ia6->ia_ifa);
dohooks(ifp->if_addrhooks, 0);
break;
+ }
- default:
- error = EOPNOTSUPP;
+err:
+ NET_UNLOCK();
+ return (error);
+}
+
+int
+in6_ioctl_get(u_long cmd, caddr_t data, struct ifnet *ifp)
+{
+ struct in6_ifreq *ifr = (struct in6_ifreq *)data;
+ struct in6_ifaddr *ia6 = NULL;
+ struct sockaddr_in6 *sa6;
+ int error = 0;
+
+ sa6 = &ifr->ifr_addr;
+
+ NET_RLOCK();
+
+ if (sa6 && sa6->sin6_family == AF_INET6) {
+ if (IN6_IS_ADDR_LINKLOCAL(&sa6->sin6_addr)) {
+ if (sa6->sin6_addr.s6_addr16[1] == 0) {
+ /* link ID is not embedded by the user */
+ sa6->sin6_addr.s6_addr16[1] =
+ htons(ifp->if_index);
+ } else if (sa6->sin6_addr.s6_addr16[1] !=
+ htons(ifp->if_index)) {
+ error = EINVAL; /* link ID contradicts */
+ goto err;
+ }
+ if (sa6->sin6_scope_id) {
+ if (sa6->sin6_scope_id !=
+ (u_int32_t)ifp->if_index) {
+ error = EINVAL;
+ goto err;
+ }
+ sa6->sin6_scope_id = 0; /* XXX: good way? */
+ }
+ }
+ ia6 = in6ifa_ifpwithaddr(ifp, &sa6->sin6_addr);
+ }
+
+ /* must think again about its semantics */
+ if (ia6 == NULL) {
+ error = EADDRNOTAVAIL;
+ goto err;
+ }
+
+ switch(cmd) {
+ case SIOCGIFDSTADDR_IN6:
+ if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
+ error = EINVAL;
+ break;
+ }
+ /*
+ * XXX: should we check if ifa_dstaddr is NULL and return
+ * an error?
+ */
+ ifr->ifr_dstaddr = ia6->ia_dstaddr;
+ break;
+
+ case SIOCGIFNETMASK_IN6:
+ ifr->ifr_addr = ia6->ia_prefixmask;
break;
+
+ case SIOCGIFAFLAG_IN6:
+ ifr->ifr_ifru.ifru_flags6 = ia6->ia6_flags;
+ break;
+
+ case SIOCGIFALIFETIME_IN6:
+ ifr->ifr_ifru.ifru_lifetime = ia6->ia6_lifetime;
+ if (ia6->ia6_lifetime.ia6t_vltime != ND6_INFINITE_LIFETIME) {
+ time_t expire, maxexpire;
+ struct in6_addrlifetime *retlt =
+ &ifr->ifr_ifru.ifru_lifetime;
+
+ /*
+ * XXX: adjust expiration time assuming time_t is
+ * signed.
+ */
+ maxexpire =
+ (time_t)~(1ULL << ((sizeof(maxexpire) * 8) - 1));
+ if (ia6->ia6_lifetime.ia6t_vltime <
+ maxexpire - ia6->ia6_updatetime) {
+ expire = ia6->ia6_updatetime +
+ ia6->ia6_lifetime.ia6t_vltime;
+ if (expire != 0) {
+ expire -= time_uptime;
+ expire += time_second;
+ }
+ retlt->ia6t_expire = expire;
+ } else
+ retlt->ia6t_expire = maxexpire;
+ }
+ if (ia6->ia6_lifetime.ia6t_pltime != ND6_INFINITE_LIFETIME) {
+ time_t expire, maxexpire;
+ struct in6_addrlifetime *retlt =
+ &ifr->ifr_ifru.ifru_lifetime;
+
+ /*
+ * XXX: adjust expiration time assuming time_t is
+ * signed.
+ */
+ maxexpire =
+ (time_t)~(1ULL << ((sizeof(maxexpire) * 8) - 1));
+ if (ia6->ia6_lifetime.ia6t_pltime <
+ maxexpire - ia6->ia6_updatetime) {
+ expire = ia6->ia6_updatetime +
+ ia6->ia6_lifetime.ia6t_pltime;
+ if (expire != 0) {
+ expire -= time_uptime;
+ expire += time_second;
+ }
+ retlt->ia6t_preferred = expire;
+ } else
+ retlt->ia6t_preferred = maxexpire;
+ }
+ break;
+
+ default:
+ panic("invalid ioctl %lu", cmd);
}
err:
- NET_UNLOCK();
+ NET_RUNLOCK();
return (error);
}
