Re: Unlock in_ioctl_get(), push kernel lock into in_ioctl_{set,change}_ifaddr()

[Klemens Nanni]

On Sat, Dec 10, 2022 at 03:40:35AM +0000, Klemens Nanni wrote:
> On Wed, Nov 30, 2022 at 06:17:51PM +0000, Klemens Nanni wrote:
> > Follow up on how in6_ioctl() does it:  grab the kernel lock in all the
> > ioctl specific functions, where needed and not earlier, i.e. exactly where
> > the net lock is currently taken/released.
> > 
> > Like in6_ioctl_get(), in_ioctl_get() simply grabs a net lock protected
> > interface address, may check net lock protected interface flags and copies
> > out data -- all under the shared net lock.
> > 
> > in_ioctl_set_ifaddr() and in_ioctl_change_ifaddr() remain kernel locked,
> > but at least their sanity check on ioctl data now happens without it.
> > 
> > Feedback? Objection? OK?
> 
> Ping.

Anyone?

IPv6 read ioctls no longer take the kernel lock, IPv4 ones still do.

Same diff from back then, no net vs. kernel lock order change, just dropping
the kernel lock on reading shared net lock protected data.

Index: netinet/in.c
===================================================================
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.179
diff -u -p -r1.179 in.c
--- netinet/in.c        6 Dec 2022 22:19:39 -0000       1.179
+++ netinet/in.c        14 Apr 2023 21:41:09 -0000
@@ -216,9 +216,7 @@ in_control(struct socket *so, u_long cmd
                break;
 #endif /* MROUTING */
        default:
-               KERNEL_LOCK();
                error = in_ioctl(cmd, data, ifp, privileged);
-               KERNEL_UNLOCK();
                break;
        }
 
@@ -262,6 +260,7 @@ in_ioctl(u_long cmd, caddr_t data, struc
                        return (error);
        }
 
+       KERNEL_LOCK();
        NET_LOCK();
 
        TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
@@ -348,6 +347,7 @@ in_ioctl(u_long cmd, caddr_t data, struc
        }
 err:
        NET_UNLOCK();
+       KERNEL_UNLOCK();
        return (error);
 }
 
@@ -372,6 +372,7 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t 
        if (error)
                return (error);
 
+       KERNEL_LOCK();
        NET_LOCK();
 
        TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
@@ -406,6 +407,7 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t 
                if_addrhooks_run(ifp);
 
        NET_UNLOCK();
+       KERNEL_UNLOCK();
        return error;
 }
 
@@ -427,6 +429,7 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
                        return (error);
        }
 
+       KERNEL_LOCK();
        NET_LOCK();
 
        TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
@@ -555,6 +558,7 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
        }
 
        NET_UNLOCK();
+       KERNEL_UNLOCK();
        return (error);
 }