On 07/11/22(Mon) 15:16, Klemens Nanni wrote:
> Not all interface ioctls need the kernel lock, but they all grab it.
>
> Here's a mechanical diff splitting the single lock/unlock around
> ifioctl() into individual lock/unlock dances inside ifioctl().
>
> From there we can unlock individual ioctls piece by piece.
>
> Survives regress on sparc64 and didn't blow up on my amd64 notebook yet.
>
> Feedback? Objection? OK?
Makes sense. Your diff is missing the kern/sys_socket.c chunk.
This stuff is hairy. I'd suggest moving very very carefully. For
example, I wouldn't bother releasing the KERNEL_LOCK() before the
if_put(). Yes, what you're suggesting is correct. Or at least should
be...
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.665
> diff -u -p -r1.665 if.c
> --- net/if.c 8 Sep 2022 10:22:06 -0000 1.665
> +++ net/if.c 7 Nov 2022 15:13:01 -0000
> @@ -1942,19 +1942,25 @@ ifioctl(struct socket *so, u_long cmd, c
> case SIOCIFCREATE:
> if ((error = suser(p)) != 0)
> return (error);
> + KERNEL_LOCK();
> error = if_clone_create(ifr->ifr_name, 0);
> + KERNEL_UNLOCK();
> return (error);
> case SIOCIFDESTROY:
> if ((error = suser(p)) != 0)
> return (error);
> + KERNEL_LOCK();
> error = if_clone_destroy(ifr->ifr_name);
> + KERNEL_UNLOCK();
> return (error);
> case SIOCSIFGATTR:
> if ((error = suser(p)) != 0)
> return (error);
> + KERNEL_LOCK();
> NET_LOCK();
> error = if_setgroupattribs(data);
> NET_UNLOCK();
> + KERNEL_UNLOCK();
> return (error);
> case SIOCGIFCONF:
> case SIOCIFGCLONERS:
> @@ -1973,12 +1979,19 @@ ifioctl(struct socket *so, u_long cmd, c
> case SIOCGIFRDOMAIN:
> case SIOCGIFGROUP:
> case SIOCGIFLLPRIO:
> - return (ifioctl_get(cmd, data));
> + KERNEL_LOCK();
> + error = ifioctl_get(cmd, data);
> + KERNEL_UNLOCK();
> + return (error);
> }
>
> + KERNEL_LOCK();
> +
> ifp = if_unit(ifr->ifr_name);
> - if (ifp == NULL)
> + if (ifp == NULL) {
> + KERNEL_UNLOCK();
> return (ENXIO);
> + }
> oif_flags = ifp->if_flags;
> oif_xflags = ifp->if_xflags;
>
> @@ -2396,6 +2409,8 @@ forceup:
>
> if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0)
> getmicrotime(&ifp->if_lastchange);
> +
> + KERNEL_UNLOCK();
>
> if_put(ifp);
>
>
