Re: in6_ioctl(): use read lock for SIOCGIF*_IN6
On 3.5.2018. 1:53, Theo Buehler wrote:
> 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.
still stable :)
Re: in6_ioctl(): use read lock for SIOCGIF*_IN6
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 - 1.223
+++ sys/netinet6/in6.c 2 May 2018 23:10:05 -
@@ -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;
-
Re: in6_ioctl(): use read lock for SIOCGIF*_IN6
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
in6_ioctl(): use read lock for SIOCGIF*_IN6
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.
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 - 1.223
+++ sys/netinet6/in6.c 2 May 2018 10:05:13 -
@@ -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,90 +311,9 @@ 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
