I like to have current "error =" notation for both mrt6_ioctl()
and in6_ioctl() within in6_control().
Also, `data’ passed to in6_ioctl_change_ifaddr() is the local
variable, kernel lock could be pushed deep down, just before
netlock.
> On 29 Nov 2022, at 16:35, Klemens Nanni <k...@openbsd.org> wrote:
>
> On Wed, Nov 23, 2022 at 03:39:54PM +0300, Vitaliy Makkoveev wrote:
>> Could this be merged with the following non "Mechanical move" diff?
>
> Here's a rebased and cleaned up diff.
>
> Feedback? Objection? OK?
>
> ---
> Neighbour Discovery information is protected by the net lock, as
> documented in nd6.h struct nd_ifinfo.
>
> ndp(8) is the only SIOCGIFINFO_IN6 and SIOCGNBRINFO_IN6 user in base.
>
> nd6_lookup(), also used in ICMP6 input and IPv6 forwarding, only needs
> the net lock.
>
> diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c
> index 44dfa3488ef..ad680278210 100644
> --- a/sys/netinet6/in6.c
> +++ b/sys/netinet6/in6.c
> @@ -205,48 +205,48 @@ in6_control(struct socket *so, u_long cmd, caddr_t
> data, struct ifnet *ifp)
>
> switch (cmd) {
> #ifdef MROUTING
> case SIOCGETSGCNT_IN6:
> case SIOCGETMIFCNT_IN6:
> KERNEL_LOCK();
> error = mrt6_ioctl(so, cmd, data);
> KERNEL_UNLOCK();
> - break;
> + return (error);
> #endif /* MROUTING */
> default:
> - KERNEL_LOCK();
> - error = in6_ioctl(cmd, data, ifp, privileged);
> - KERNEL_UNLOCK();
> - break;
> + return (in6_ioctl(cmd, data, ifp, privileged));
> }
> -
> - return error;
> }
>
> int
> in6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
> {
> + int error;
> +
> if (ifp == NULL)
> return (ENXIO);
>
> switch (cmd) {
> case SIOCGIFINFO_IN6:
> case SIOCGNBRINFO_IN6:
> return (nd6_ioctl(cmd, data, ifp));
> case SIOCGIFDSTADDR_IN6:
> case SIOCGIFNETMASK_IN6:
> case SIOCGIFAFLAG_IN6:
> case SIOCGIFALIFETIME_IN6:
> return (in6_ioctl_get(cmd, data, ifp));
> case SIOCAIFADDR_IN6:
> case SIOCDIFADDR_IN6:
> if (!privileged)
> return (EPERM);
> - return (in6_ioctl_change_ifaddr(cmd, data, ifp));
> + KERNEL_LOCK();
> + error = in6_ioctl_change_ifaddr(cmd, data, ifp);
> + KERNEL_UNLOCK();
> + return (error);
> case SIOCSIFADDR:
> case SIOCSIFDSTADDR:
> case SIOCSIFBRDADDR:
> case SIOCSIFNETMASK:
> /*
> * Do not pass those ioctl to driver handler since they are not
> * properly set up. Instead just error out.
> */
> @@ -417,16 +417,17 @@ in6_ioctl_get(u_long cmd, caddr_t data, struct ifnet
> *ifp)
> sa = sin6tosa(&ifr->ifr_addr);
> if (sa->sa_family == AF_INET6) {
> sa->sa_len = sizeof(struct sockaddr_in6);
> error = in6_sa2sin6(sa, &sa6);
> if (error)
> return (error);
> }
>
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
>
> if (sa6 != NULL) {
> error = in6_check_embed_scope(sa6, ifp->if_index);
> if (error)
> goto err;
> error = in6_clear_scope_id(sa6, ifp->if_index);
> if (error)
> @@ -512,16 +513,17 @@ in6_ioctl_get(u_long cmd, caddr_t data, struct ifnet
> *ifp)
> break;
>
> default:
> panic("%s: invalid ioctl %lu", __func__, cmd);
> }
>
> err:
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> }
>
> int
> in6_check_embed_scope(struct sockaddr_in6 *sa6, unsigned int ifidx)
> {
> if (IN6_IS_ADDR_LINKLOCAL(&sa6->sin6_addr)) {
> if (sa6->sin6_addr.s6_addr16[1] == 0) {
>
