On Wed, Nov 01, 2017 at 06:03:26PM +0000, Martin Pieuchot wrote:
> ifioctl() contains two fallthrough paths that end up in ifp->if_ioctl().
> The diff below merges them.
>
> But instead of calling ifp->if_ioctl() from inside in{,6}_ioctl(), I
> changed the logic to return EOPNOTSUPP. The idea is that if_ioctl()
> is part of the driver and will need a different lock than the address
> layers.
The diff reads OK. But I don't understand why you go pr_usrreq ->
in_control -> in_ioctl to end up back in ifioctl to then finally go to
the driver on EOPNOTSUPP.
I seem to be missing something, why can't we list the ioctls in ifictl
that should go to the driver and skip the detour through netinet{,6}?
>
> Along the way I unified the error codes returned when `ifp' is NULL.
> Note that this should never happen in in{,6}_ioctl(), but that's a
> different crusade.
>
> Comments, oks?
>
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.521
> diff -u -p -r1.521 if.c
> --- net/if.c 31 Oct 2017 22:05:12 -0000 1.521
> +++ net/if.c 1 Nov 2017 17:13:26 -0000
> @@ -1985,30 +1985,6 @@ ifioctl(struct socket *so, u_long cmd, c
> rtm_ifchg(ifp);
> break;
>
> - case SIOCDIFPHYADDR:
> - case SIOCSLIFPHYADDR:
> - case SIOCSLIFPHYRTABLE:
> - case SIOCSLIFPHYTTL:
> - case SIOCADDMULTI:
> - case SIOCDELMULTI:
> - case SIOCSIFMEDIA:
> - case SIOCSVNETID:
> - case SIOCSIFPAIR:
> - case SIOCSIFPARENT:
> - case SIOCDIFPARENT:
> - if ((error = suser(p, 0)) != 0)
> - break;
> - /* FALLTHROUGH */
> - case SIOCGLIFPHYADDR:
> - case SIOCGLIFPHYRTABLE:
> - case SIOCGLIFPHYTTL:
> - case SIOCGIFMEDIA:
> - case SIOCGVNETID:
> - case SIOCGIFPAIR:
> - case SIOCGIFPARENT:
> - error = (*ifp->if_ioctl)(ifp, cmd, data);
> - break;
> -
> case SIOCGIFDESCR:
> strlcpy(ifdescrbuf, ifp->if_description, IFDESCRSIZE);
> error = copyoutstr(ifdescrbuf, ifr->ifr_data, IFDESCRSIZE,
> @@ -2138,10 +2114,26 @@ ifioctl(struct socket *so, u_long cmd, c
> ifp->if_llprio = ifr->ifr_llprio;
> break;
>
> + case SIOCDIFPHYADDR:
> + case SIOCSLIFPHYADDR:
> + case SIOCSLIFPHYRTABLE:
> + case SIOCSLIFPHYTTL:
> + case SIOCADDMULTI:
> + case SIOCDELMULTI:
> + case SIOCSIFMEDIA:
> + case SIOCSVNETID:
> + case SIOCSIFPAIR:
> + case SIOCSIFPARENT:
> + case SIOCDIFPARENT:
> + if ((error = suser(p, 0)) != 0)
> + break;
> + /* FALLTHROUGH */
> default:
> error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL,
> (struct mbuf *) cmd, (struct mbuf *) data,
> (struct mbuf *) ifp, p));
> + if (error == EOPNOTSUPP)
> + error = ((*ifp->if_ioctl)(ifp, cmd, data));
> break;
> }
>
> Index: netinet/in.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in.c,v
> retrieving revision 1.143
> diff -u -p -r1.143 in.c
> --- netinet/in.c 24 Oct 2017 09:30:15 -0000 1.143
> +++ netinet/in.c 1 Nov 2017 17:13:26 -0000
> @@ -213,7 +213,7 @@ in_ioctl(u_long cmd, caddr_t data, struc
> int newifaddr;
>
> if (ifp == NULL)
> - return (EOPNOTSUPP);
> + return (ENXIO);
>
> NET_ASSERT_LOCKED();
>
> @@ -393,8 +393,7 @@ in_ioctl(u_long cmd, caddr_t data, struc
> break;
>
> default:
> - error = ((*ifp->if_ioctl)(ifp, cmd, data));
> - return (error);
> + return (EOPNOTSUPP);
> }
> return (0);
> }
> Index: netinet6/in6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.216
> diff -u -p -r1.216 in6.c
> --- netinet6/in6.c 26 Oct 2017 15:05:41 -0000 1.216
> +++ netinet6/in6.c 1 Nov 2017 17:13:26 -0000
> @@ -207,10 +207,11 @@ in6_ioctl(u_long cmd, caddr_t data, stru
> struct in6_ifaddr *ia6 = NULL;
> struct in6_aliasreq *ifra = (struct in6_aliasreq *)data;
> struct sockaddr_in6 *sa6;
> - int error;
>
> if (ifp == NULL)
> - return (EOPNOTSUPP);
> + return (ENXIO);
> +
> + NET_ASSERT_LOCKED();
>
> switch (cmd) {
> case SIOCGIFINFO_IN6:
> @@ -249,7 +250,7 @@ in6_ioctl(u_long cmd, caddr_t data, stru
> * Do not pass those ioctl to driver handler since they are not
> * properly setup. Instead just error out.
> */
> - return (EOPNOTSUPP);
> + return (EINVAL);
> default:
> sa6 = NULL;
> break;
> @@ -311,7 +312,6 @@ in6_ioctl(u_long cmd, caddr_t data, stru
> }
>
> switch (cmd) {
> -
> case SIOCGIFDSTADDR_IN6:
> if ((ifp->if_flags & IFF_POINTOPOINT) == 0)
> return (EINVAL);
> @@ -454,8 +454,7 @@ in6_ioctl(u_long cmd, caddr_t data, stru
> break;
>
> default:
> - error = ((*ifp->if_ioctl)(ifp, cmd, data));
> - return (error);
> + return (EOPNOTSUPP);
> }
>
> return (0);
>
--
I'm not entirely sure you are real.
