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;
