Re: ifioctl() cleanup, step 2
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 - 1.514
> +++ net/if.c 11 Oct 2017 08:37:31 -
> @@ -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, );
> 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,
>
Re: ifioctl() cleanup, step 2
On Wed, Oct 11, 2017 at 12:43:45PM +0200, Martin Pieuchot wrote: > On 11/10/17(Wed) 12:28, Alexandr Nedvedicky wrote: > > Hello, > > > > just for my curiosity: > > > > why do you leave some returns untreated? is that intentional? > > Yes it is intentional to make the diff shorter and easier to review. > O.K. understood. thanks and regards sasha
Re: ifioctl() cleanup, step 2
On 11/10/17(Wed) 12:28, Alexandr Nedvedicky wrote:
> Hello,
>
> just for my curiosity:
>
> why do you leave some returns untreated? is that intentional?
Yes it is intentional to make the diff shorter and easier to review.
> just like here:
> @@ -2048,8 +2048,11 @@ ifioctl(struct socket *so, u_long cmd, caddr_t
> data, struct proc *p)
> case SIOCGVNETID:
> case SIOCGIFPAIR:
> case SIOCGIFPARENT:
> - if (ifp->if_ioctl == 0)
> - return (EOPNOTSUPP);
> + if (ifp->if_ioctl == 0) {
> + /* ??? sashan@ */
> + error = EOPNOTSUPP;
> + break;
> + }
> error = (*ifp->if_ioctl)(ifp, cmd, data);
> break;
This will be addressed in the next diff. We can simply remove the if ()
block because all interface MUST have an if_ioctl handler.
Re: ifioctl() cleanup, step 2
Hello,
just for my curiosity:
why do you leave some returns untreated? is that intentional?
just like here:
@@ -2048,8 +2048,11 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
case SIOCGVNETID:
case SIOCGIFPAIR:
case SIOCGIFPARENT:
- if (ifp->if_ioctl == 0)
- return (EOPNOTSUPP);
+ if (ifp->if_ioctl == 0) {
+ /* ??? sashan@ */
+ error = EOPNOTSUPP;
+ break;
+ }
error = (*ifp->if_ioctl)(ifp, cmd, data);
break;
below is patch, which makes switch in ifioctl() consistent by converting
all return (error) to breaks;
thanks and
regards
sasha
8<---8<---8<--8<
diff --git a/sys/net/if.c b/sys/net/if.c
index 60020606df5..40f6fe1b776 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1876,7 +1876,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
break;
#endif /* INET6 */
default:
- return (EAFNOSUPPORT);
+ error = EAFNOSUPPORT;
}
if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
rtm_ifchg(ifp);
@@ -1920,7 +1920,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
case SIOCSIFFLAGS:
if ((error = suser(p, 0)) != 0)
- return (error);
+ break;
ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) |
(ifr->ifr_flags & ~IFF_CANTCHANGE);
@@ -1945,13 +1945,13 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
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 */
@@ -1983,7 +1983,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
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)) {
@@ -1992,7 +1992,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
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;
@@ -2007,13 +2007,13 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
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);
@@ -2037,7 +2037,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
case SIOCSIFPARENT:
case SIOCDIFPARENT:
if ((error = suser(p, 0)) != 0)
- return (error);
+ break;
/* FALLTHROUGH */
case SIOCGIFPSRCADDR:
case SIOCGIFPDSTADDR:
@@ -2048,8 +2048,10 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
case SIOCGVNETID:
case SIOCGIFPAIR:
case SIOCGIFPARENT:
- if (ifp->if_ioctl == 0)
- return (EOPNOTSUPP);
+ if (ifp->if_ioctl == 0) {
+ error = EOPNOTSUPP;
+ break;
+ }
error = (*ifp->if_ioctl)(ifp, cmd, data);
break;
@@ -2061,7 +2063,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
case SIOCSIFDESCR:
if ((error = suser(p, 0)) != 0)
- return (error);
+ break;
error =
ifioctl() cleanup, step 2
Moar cleanup to be able to selectively take the NET_LOCK() around some
ioctls.
This diff change many "return (error)" into "break".
It adds error checks for SIOC{A,D}IFGROUP. The only driver handling
these ioctl(2)s is... carp(4)!
ok?
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.c11 Oct 2017 07:57:27 - 1.514
+++ net/if.c11 Oct 2017 08:37:31 -
@@ -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, );
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, );
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
Re: ifioctl() cleanup
On Tue, Oct 10, 2017 at 03:55:30PM +0200, Martin Pieuchot wrote:
> Similar diff without factorizing privilege checks, deraadt@ pointed out
> it is too fragile.
>
> ok?
OK bluhm@
>
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.513
> diff -u -p -r1.513 if.c
> --- net/if.c 9 Oct 2017 08:35:38 - 1.513
> +++ net/if.c 10 Oct 2017 13:46:43 -
> @@ -1816,10 +1816,9 @@ int
> ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> {
> struct ifnet *ifp;
> - struct ifreq *ifr;
> - struct sockaddr_dl *sdl;
> + struct ifreq *ifr = (struct ifreq *)data;
> struct ifgroupreq *ifgr;
> - struct if_afreq *ifar;
> + struct if_afreq *ifar = (struct if_afreq *)data;
> char ifdescrbuf[IFDESCRSIZE];
> char ifrtlabelbuf[RTLABEL_LEN];
> int s, error = 0, oif_xflags;
> @@ -1828,13 +1827,8 @@ ifioctl(struct socket *so, u_long cmd, c
> const char *label;
>
> switch (cmd) {
> -
> case SIOCGIFCONF:
> return (ifconf(cmd, data));
> - }
> - ifr = (struct ifreq *)data;
> -
> - switch (cmd) {
> case SIOCIFCREATE:
> case SIOCIFDESTROY:
> if ((error = suser(p, 0)) != 0)
> @@ -1852,15 +1846,19 @@ ifioctl(struct socket *so, u_long cmd, c
> if ((error = suser(p, 0)) != 0)
> return (error);
> return (if_setgroupattribs(data));
> + }
> +
> + ifp = ifunit(ifr->ifr_name);
> + if (ifp == NULL)
> + return (ENXIO);
> + oif_flags = ifp->if_flags;
> + oif_xflags = ifp->if_xflags;
> +
> + switch (cmd) {
> case SIOCIFAFATTACH:
> case SIOCIFAFDETACH:
> if ((error = suser(p, 0)) != 0)
> return (error);
> - ifar = (struct if_afreq *)data;
> - if ((ifp = ifunit(ifar->ifar_name)) == NULL)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
> - oif_xflags = ifp->if_xflags;
> switch (ifar->ifar_af) {
> case AF_INET:
> /* attach is a noop for AF_INET */
> @@ -1878,16 +1876,7 @@ ifioctl(struct socket *so, u_long cmd, c
> default:
> return (EAFNOSUPPORT);
> }
> - if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
> - rtm_ifchg(ifp);
> - return (error);
> - }
> -
> - ifp = ifunit(ifr->ifr_name);
> - if (ifp == 0)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
> - switch (cmd) {
> + break;
>
> case SIOCGIFFLAGS:
> ifr->ifr_flags = ifp->if_flags;
> @@ -2002,7 +1991,6 @@ ifioctl(struct socket *so, u_long cmd, c
>
> ifp->if_xflags = (ifp->if_xflags & IFXF_CANTCHANGE) |
> (ifr->ifr_flags & ~IFXF_CANTCHANGE);
> - rtm_ifchg(ifp);
> break;
>
> case SIOCSIFMETRIC:
> @@ -2140,8 +2128,7 @@ ifioctl(struct socket *so, u_long cmd, c
> case SIOCSIFLLADDR:
> if ((error = suser(p, 0)))
> return (error);
> - sdl = ifp->if_sadl;
> - if (sdl == NULL)
> + if (ifp->if_sadl == NULL)
> return (EINVAL);
> if (ifr->ifr_addr.sa_len != ETHER_ADDR_LEN)
> return (EINVAL);
> @@ -2182,6 +2169,9 @@ ifioctl(struct socket *so, u_long cmd, c
> (struct mbuf *) ifp, p));
> break;
> }
> +
> + if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
> + rtm_ifchg(ifp);
>
> if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0)
> getmicrotime(>if_lastchange);
Re: ifioctl() cleanup
On 09/10/17(Mon) 16:34, Martin Pieuchot wrote:
> Here's a small cleanup to make it easy to push the NET_LOCK() further
> down.
>
> Diff below factorize all ioctl requests need root privileges in one
> switch () block.
>
> It moves SIOCIFAFATTACH/SIOCIFAFDETACH where it belongs, to the block
> that keeps track of the timestamps of the last change.
>
> It gets rid of unnecessary `sdl' variable.
Similar diff without factorizing privilege checks, deraadt@ pointed out
it is too fragile.
ok?
Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.513
diff -u -p -r1.513 if.c
--- net/if.c9 Oct 2017 08:35:38 - 1.513
+++ net/if.c10 Oct 2017 13:46:43 -
@@ -1816,10 +1816,9 @@ int
ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
{
struct ifnet *ifp;
- struct ifreq *ifr;
- struct sockaddr_dl *sdl;
+ struct ifreq *ifr = (struct ifreq *)data;
struct ifgroupreq *ifgr;
- struct if_afreq *ifar;
+ struct if_afreq *ifar = (struct if_afreq *)data;
char ifdescrbuf[IFDESCRSIZE];
char ifrtlabelbuf[RTLABEL_LEN];
int s, error = 0, oif_xflags;
@@ -1828,13 +1827,8 @@ ifioctl(struct socket *so, u_long cmd, c
const char *label;
switch (cmd) {
-
case SIOCGIFCONF:
return (ifconf(cmd, data));
- }
- ifr = (struct ifreq *)data;
-
- switch (cmd) {
case SIOCIFCREATE:
case SIOCIFDESTROY:
if ((error = suser(p, 0)) != 0)
@@ -1852,15 +1846,19 @@ ifioctl(struct socket *so, u_long cmd, c
if ((error = suser(p, 0)) != 0)
return (error);
return (if_setgroupattribs(data));
+ }
+
+ ifp = ifunit(ifr->ifr_name);
+ if (ifp == NULL)
+ return (ENXIO);
+ oif_flags = ifp->if_flags;
+ oif_xflags = ifp->if_xflags;
+
+ switch (cmd) {
case SIOCIFAFATTACH:
case SIOCIFAFDETACH:
if ((error = suser(p, 0)) != 0)
return (error);
- ifar = (struct if_afreq *)data;
- if ((ifp = ifunit(ifar->ifar_name)) == NULL)
- return (ENXIO);
- oif_flags = ifp->if_flags;
- oif_xflags = ifp->if_xflags;
switch (ifar->ifar_af) {
case AF_INET:
/* attach is a noop for AF_INET */
@@ -1878,16 +1876,7 @@ ifioctl(struct socket *so, u_long cmd, c
default:
return (EAFNOSUPPORT);
}
- if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
- rtm_ifchg(ifp);
- return (error);
- }
-
- ifp = ifunit(ifr->ifr_name);
- if (ifp == 0)
- return (ENXIO);
- oif_flags = ifp->if_flags;
- switch (cmd) {
+ break;
case SIOCGIFFLAGS:
ifr->ifr_flags = ifp->if_flags;
@@ -2002,7 +1991,6 @@ ifioctl(struct socket *so, u_long cmd, c
ifp->if_xflags = (ifp->if_xflags & IFXF_CANTCHANGE) |
(ifr->ifr_flags & ~IFXF_CANTCHANGE);
- rtm_ifchg(ifp);
break;
case SIOCSIFMETRIC:
@@ -2140,8 +2128,7 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCSIFLLADDR:
if ((error = suser(p, 0)))
return (error);
- sdl = ifp->if_sadl;
- if (sdl == NULL)
+ if (ifp->if_sadl == NULL)
return (EINVAL);
if (ifr->ifr_addr.sa_len != ETHER_ADDR_LEN)
return (EINVAL);
@@ -2182,6 +2169,9 @@ ifioctl(struct socket *so, u_long cmd, c
(struct mbuf *) ifp, p));
break;
}
+
+ if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
+ rtm_ifchg(ifp);
if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0)
getmicrotime(>if_lastchange);
Re: ifioctl() cleanup
On Mon, Oct 09, 2017 at 04:34:38PM +0200, Martin Pieuchot wrote:
> Here's a small cleanup to make it easy to push the NET_LOCK() further
> down.
>
> Diff below factorize all ioctl requests need root privileges in one
> switch () block.
>
> It moves SIOCIFAFATTACH/SIOCIFAFDETACH where it belongs, to the block
> that keeps track of the timestamps of the last change.
>
> It gets rid of unnecessary `sdl' variable.
>
> ok?
Before the cmds
SIOCAIFGROUP
SIOCDIFGROUP
SIOCSIFLLADDR
SIOCSIFLLPRIO
returned EPERM, with this patch 1.. Is this a problem?
Regards,
Michele
>
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.513
> diff -u -p -r1.513 if.c
> --- net/if.c 9 Oct 2017 08:35:38 - 1.513
> +++ net/if.c 9 Oct 2017 14:27:35 -
> @@ -1816,10 +1816,9 @@ int
> ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> {
> struct ifnet *ifp;
> - struct ifreq *ifr;
> - struct sockaddr_dl *sdl;
> + struct ifreq *ifr = (struct ifreq *)data;
> struct ifgroupreq *ifgr;
> - struct if_afreq *ifar;
> + struct if_afreq *ifar = (struct if_afreq *)data;
> char ifdescrbuf[IFDESCRSIZE];
> char ifrtlabelbuf[RTLABEL_LEN];
> int s, error = 0, oif_xflags;
> @@ -1828,17 +1827,49 @@ ifioctl(struct socket *so, u_long cmd, c
> const char *label;
>
> switch (cmd) {
> -
> - case SIOCGIFCONF:
> - return (ifconf(cmd, data));
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + case SIOCSIFGATTR:
> + case SIOCIFAFATTACH:
> + case SIOCIFAFDETACH:
> + case SIOCSIFFLAGS:
> + case SIOCSIFXFLAGS:
> + case SIOCSIFMETRIC:
> + case SIOCSIFMTU:
> + case SIOCSIFPHYADDR:
> + case SIOCDIFPHYADDR:
> +#ifdef INET6
> + case SIOCSIFPHYADDR_IN6:
> +#endif
> + case SIOCSLIFPHYADDR:
> + case SIOCSLIFPHYRTABLE:
> + case SIOCSLIFPHYTTL:
> + case SIOCADDMULTI:
> + case SIOCDELMULTI:
> + case SIOCSIFMEDIA:
> + case SIOCSVNETID:
> + case SIOCSIFPAIR:
> + case SIOCSIFPARENT:
> + case SIOCDIFPARENT:
> + case SIOCSIFDESCR:
> + case SIOCSIFRTLABEL:
> + case SIOCSIFPRIORITY:
> + case SIOCSIFRDOMAIN:
> + case SIOCAIFGROUP:
> + case SIOCDIFGROUP:
> + case SIOCSIFLLADDR:
> + case SIOCSIFLLPRIO:
> + if ((error = suser(p, 0)) != 0)
> + return (error);
> + default:
> + break;
> }
> - ifr = (struct ifreq *)data;
>
> switch (cmd) {
> + case SIOCGIFCONF:
> + return (ifconf(cmd, data));
> case SIOCIFCREATE:
> case SIOCIFDESTROY:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
> return ((cmd == SIOCIFCREATE) ?
> if_clone_create(ifr->ifr_name, 0) :
> if_clone_destroy(ifr->ifr_name));
> @@ -1849,17 +1880,17 @@ ifioctl(struct socket *so, u_long cmd, c
> case SIOCGIFGATTR:
> return (if_getgroupattribs(data));
> case SIOCSIFGATTR:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
> return (if_setgroupattribs(data));
> + }
> +
> + ifp = ifunit(ifr->ifr_name);
> + if (ifp == NULL)
> + return (ENXIO);
> + oif_flags = ifp->if_flags;
> +
> + switch (cmd) {
> case SIOCIFAFATTACH:
> case SIOCIFAFDETACH:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
> - ifar = (struct if_afreq *)data;
> - if ((ifp = ifunit(ifar->ifar_name)) == NULL)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
> oif_xflags = ifp->if_xflags;
> switch (ifar->ifar_af) {
> case AF_INET:
> @@ -1880,15 +1911,7 @@ ifioctl(struct socket *so, u_long cmd, c
> }
> if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
> rtm_ifchg(ifp);
> - return (error);
> - }
> -
> - ifp = ifunit(ifr->ifr_name);
> - if (ifp == 0)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
> - switch (cmd) {
> -
> + break;
> case SIOCGIFFLAGS:
> ifr->ifr_flags = ifp->if_flags;
> if (ifq_is_oactive(>if_snd))
> @@ -1919,9 +1942,6 @@ ifioctl(struct socket *so, u_long cmd, c
> }
>
> case SIOCSIFFLAGS:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
> -
> ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) |
> (ifr->ifr_flags & ~IFF_CANTCHANGE);
>
> @@ -1944,9 +1964,6 @@ ifioctl(struct socket *so, u_long cmd, c
> break;
>
> case SIOCSIFXFLAGS:
> - if ((error = suser(p, 0)) != 0)
> -
ifioctl() cleanup
Here's a small cleanup to make it easy to push the NET_LOCK() further
down.
Diff below factorize all ioctl requests need root privileges in one
switch () block.
It moves SIOCIFAFATTACH/SIOCIFAFDETACH where it belongs, to the block
that keeps track of the timestamps of the last change.
It gets rid of unnecessary `sdl' variable.
ok?
Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.513
diff -u -p -r1.513 if.c
--- net/if.c9 Oct 2017 08:35:38 - 1.513
+++ net/if.c9 Oct 2017 14:27:35 -
@@ -1816,10 +1816,9 @@ int
ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
{
struct ifnet *ifp;
- struct ifreq *ifr;
- struct sockaddr_dl *sdl;
+ struct ifreq *ifr = (struct ifreq *)data;
struct ifgroupreq *ifgr;
- struct if_afreq *ifar;
+ struct if_afreq *ifar = (struct if_afreq *)data;
char ifdescrbuf[IFDESCRSIZE];
char ifrtlabelbuf[RTLABEL_LEN];
int s, error = 0, oif_xflags;
@@ -1828,17 +1827,49 @@ ifioctl(struct socket *so, u_long cmd, c
const char *label;
switch (cmd) {
-
- case SIOCGIFCONF:
- return (ifconf(cmd, data));
+ case SIOCIFCREATE:
+ case SIOCIFDESTROY:
+ case SIOCSIFGATTR:
+ case SIOCIFAFATTACH:
+ case SIOCIFAFDETACH:
+ case SIOCSIFFLAGS:
+ case SIOCSIFXFLAGS:
+ case SIOCSIFMETRIC:
+ case SIOCSIFMTU:
+ case SIOCSIFPHYADDR:
+ case SIOCDIFPHYADDR:
+#ifdef INET6
+ case SIOCSIFPHYADDR_IN6:
+#endif
+ case SIOCSLIFPHYADDR:
+ case SIOCSLIFPHYRTABLE:
+ case SIOCSLIFPHYTTL:
+ case SIOCADDMULTI:
+ case SIOCDELMULTI:
+ case SIOCSIFMEDIA:
+ case SIOCSVNETID:
+ case SIOCSIFPAIR:
+ case SIOCSIFPARENT:
+ case SIOCDIFPARENT:
+ case SIOCSIFDESCR:
+ case SIOCSIFRTLABEL:
+ case SIOCSIFPRIORITY:
+ case SIOCSIFRDOMAIN:
+ case SIOCAIFGROUP:
+ case SIOCDIFGROUP:
+ case SIOCSIFLLADDR:
+ case SIOCSIFLLPRIO:
+ if ((error = suser(p, 0)) != 0)
+ return (error);
+ default:
+ break;
}
- ifr = (struct ifreq *)data;
switch (cmd) {
+ case SIOCGIFCONF:
+ return (ifconf(cmd, data));
case SIOCIFCREATE:
case SIOCIFDESTROY:
- if ((error = suser(p, 0)) != 0)
- return (error);
return ((cmd == SIOCIFCREATE) ?
if_clone_create(ifr->ifr_name, 0) :
if_clone_destroy(ifr->ifr_name));
@@ -1849,17 +1880,17 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCGIFGATTR:
return (if_getgroupattribs(data));
case SIOCSIFGATTR:
- if ((error = suser(p, 0)) != 0)
- return (error);
return (if_setgroupattribs(data));
+ }
+
+ ifp = ifunit(ifr->ifr_name);
+ if (ifp == NULL)
+ return (ENXIO);
+ oif_flags = ifp->if_flags;
+
+ switch (cmd) {
case SIOCIFAFATTACH:
case SIOCIFAFDETACH:
- if ((error = suser(p, 0)) != 0)
- return (error);
- ifar = (struct if_afreq *)data;
- if ((ifp = ifunit(ifar->ifar_name)) == NULL)
- return (ENXIO);
- oif_flags = ifp->if_flags;
oif_xflags = ifp->if_xflags;
switch (ifar->ifar_af) {
case AF_INET:
@@ -1880,15 +1911,7 @@ ifioctl(struct socket *so, u_long cmd, c
}
if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
rtm_ifchg(ifp);
- return (error);
- }
-
- ifp = ifunit(ifr->ifr_name);
- if (ifp == 0)
- return (ENXIO);
- oif_flags = ifp->if_flags;
- switch (cmd) {
-
+ break;
case SIOCGIFFLAGS:
ifr->ifr_flags = ifp->if_flags;
if (ifq_is_oactive(>if_snd))
@@ -1919,9 +1942,6 @@ ifioctl(struct socket *so, u_long cmd, c
}
case SIOCSIFFLAGS:
- if ((error = suser(p, 0)) != 0)
- return (error);
-
ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) |
(ifr->ifr_flags & ~IFF_CANTCHANGE);
@@ -1944,9 +1964,6 @@ ifioctl(struct socket *so, u_long cmd, c
break;
case SIOCSIFXFLAGS:
- if ((error = suser(p, 0)) != 0)
- return (error);
-
#ifdef INET6
if (ISSET(ifr->ifr_flags, IFXF_AUTOCONF6)) {
error = in6_ifattach(ifp);
@@ -2006,14 +2023,10 @@ ifioctl(struct socket *so, u_long cmd, c
break;
case SIOCSIFMETRIC:
- if ((error = suser(p, 0)) != 0)
-
