On Mon, Nov 07, 2022 at 03:48:46PM +0000, Martin Pieuchot wrote:
> 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.
Oops, thanks.
> 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...
Yup, best to do this in small steps.
kernel unlock before if_put is fine, other places also do it, I'd prefer
to communicate this here as well by doing it in that order.
Feedback? OK?
Index: kern/sys_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_socket.c,v
retrieving revision 1.54
diff -u -p -r1.54 sys_socket.c
--- kern/sys_socket.c 2 Sep 2022 13:12:31 -0000 1.54
+++ kern/sys_socket.c 7 Nov 2022 15:59:33 -0000
@@ -129,9 +129,7 @@ soo_ioctl(struct file *fp, u_long cmd, c
* different entry since a socket's unnecessary
*/
if (IOCGROUP(cmd) == 'i') {
- KERNEL_LOCK();
error = ifioctl(so, cmd, data, p);
- KERNEL_UNLOCK();
return (error);
}
if (IOCGROUP(cmd) == 'r')
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);
