On Wed, Oct 11, 2017 at 10:44:22AM +0200, Martin Pieuchot wrote: > Moar cleanup to be able to selectively take the NET_LOCK() around some > ioctls. > > This diff change many "return (error)" into "break".
It looks a bit inconsistent, where you replace the return and where not. But I am sure your next diff will resolve this. > It adds error checks for SIOC{A,D}IFGROUP. The only driver handling > these ioctl(2)s is... carp(4)! This (error == ENOTTY) is strange. I hides something in the kernel where the user land requests impossible things and cannot cope with an error. But as we do not want to break things, I think this approach makes sense. > ok? OK bluhm@ > Index: net/if.c > =================================================================== > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.514 > diff -u -p -r1.514 if.c > --- net/if.c 11 Oct 2017 07:57:27 -0000 1.514 > +++ net/if.c 11 Oct 2017 08:37:31 -0000 > @@ -1817,7 +1817,7 @@ ifioctl(struct socket *so, u_long cmd, c > { > struct ifnet *ifp; > struct ifreq *ifr = (struct ifreq *)data; > - struct ifgroupreq *ifgr; > + struct ifgroupreq *ifgr = (struct ifgroupreq *)data; > struct if_afreq *ifar = (struct if_afreq *)data; > char ifdescrbuf[IFDESCRSIZE]; > char ifrtlabelbuf[RTLABEL_LEN]; > @@ -1858,7 +1858,7 @@ ifioctl(struct socket *so, u_long cmd, c > case SIOCIFAFATTACH: > case SIOCIFAFDETACH: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > switch (ifar->ifar_af) { > case AF_INET: > /* attach is a noop for AF_INET */ > @@ -1874,7 +1874,7 @@ ifioctl(struct socket *so, u_long cmd, c > break; > #endif /* INET6 */ > default: > - return (EAFNOSUPPORT); > + error = EAFNOSUPPORT; > } > break; > > @@ -1909,7 +1909,7 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFFLAGS: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > > ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) | > (ifr->ifr_flags & ~IFF_CANTCHANGE); > @@ -1934,13 +1934,13 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFXFLAGS: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > > #ifdef INET6 > if (ISSET(ifr->ifr_flags, IFXF_AUTOCONF6)) { > error = in6_ifattach(ifp); > if (error != 0) > - return (error); > + break; > } > #endif /* INET6 */ > > @@ -1972,7 +1972,7 @@ ifioctl(struct socket *so, u_long cmd, c > error = ifp->if_wol(ifp, 1); > splx(s); > if (error) > - return (error); > + break; > } > if (ISSET(ifp->if_xflags, IFXF_WOL) && > !ISSET(ifr->ifr_flags, IFXF_WOL)) { > @@ -1981,7 +1981,7 @@ ifioctl(struct socket *so, u_long cmd, c > error = ifp->if_wol(ifp, 0); > splx(s); > if (error) > - return (error); > + break; > } > } else if (ISSET(ifr->ifr_flags, IFXF_WOL)) { > ifr->ifr_flags &= ~IFXF_WOL; > @@ -1995,13 +1995,13 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFMETRIC: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > ifp->if_metric = ifr->ifr_metric; > break; > > case SIOCSIFMTU: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > if (ifp->if_ioctl == NULL) > return (EOPNOTSUPP); > error = (*ifp->if_ioctl)(ifp, cmd, data); > @@ -2049,7 +2049,7 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFDESCR: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > error = copyinstr(ifr->ifr_data, ifdescrbuf, > IFDESCRSIZE, &bytesdone); > if (error == 0) { > @@ -2070,7 +2070,7 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFRTLABEL: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > error = copyinstr(ifr->ifr_data, ifrtlabelbuf, > RTLABEL_LEN, &bytesdone); > if (error == 0) { > @@ -2085,7 +2085,7 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFPRIORITY: > if ((error = suser(p, 0)) != 0) > - return (error); > + break; > if (ifr->ifr_metric < 0 || ifr->ifr_metric > 15) > return (EINVAL); > ifp->if_priority = ifr->ifr_metric; > @@ -2097,32 +2097,32 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFRDOMAIN: > if ((error = suser(p, 0)) != 0) > - return (error); > - if ((error = if_setrdomain(ifp, ifr->ifr_rdomainid)) != 0) > - return (error); > + break; > + error = if_setrdomain(ifp, ifr->ifr_rdomainid); > break; > > case SIOCAIFGROUP: > if ((error = suser(p, 0))) > - return (error); > - ifgr = (struct ifgroupreq *)data; > + break; > if ((error = if_addgroup(ifp, ifgr->ifgr_group))) > - return (error); > - (*ifp->if_ioctl)(ifp, cmd, data); /* XXX error check */ > + break; > + error = (*ifp->if_ioctl)(ifp, cmd, data); > + if (error == ENOTTY) > + error = 0; > break; > > case SIOCGIFGROUP: > - if ((error = if_getgroup(data, ifp))) > - return (error); > + error = if_getgroup(data, ifp); > break; > > case SIOCDIFGROUP: > if ((error = suser(p, 0))) > - return (error); > - (*ifp->if_ioctl)(ifp, cmd, data); /* XXX error check */ > - ifgr = (struct ifgroupreq *)data; > - if ((error = if_delgroup(ifp, ifgr->ifgr_group))) > - return (error); > + break; > + error = (*ifp->if_ioctl)(ifp, cmd, data); > + if (error == ENOTTY) > + error = 0; > + if (error == 0) > + error = if_delgroup(ifp, ifgr->ifgr_group); > break; > > case SIOCSIFLLADDR: > @@ -2157,7 +2157,7 @@ ifioctl(struct socket *so, u_long cmd, c > > case SIOCSIFLLPRIO: > if ((error = suser(p, 0))) > - return (error); > + break; > if (ifr->ifr_llprio > UCHAR_MAX) > return (EINVAL); > ifp->if_llprio = ifr->ifr_llprio;