per-net rpc shutdown

Reviewing your more recent patches I think we have a problem with some of the code that's already merged. See the comment in svc_shutdown_net:

	void svc_shutdown_net(struct svc_serv *serv, struct net *net)
	{               

/*
* The set of xprts (contained in the sv_tempsocks and
* sv_permsocks lists) is now constant, since it is modified
* only by accepting new sockets (done by service threads in
* svc_recv) or aging old ones (done by sv_temptimer), or
* configuration changes (excluded by whatever locking the
* caller is using--nfsd_mutex in the case of nfsd).

		   So it's
		 * safe to traverse those lists and shut everything down:
		 */       
		svc_close_net(serv, net);
	
		if (serv->sv_shutdown)
			serv->sv_shutdown(serv, net);
	}

So we depend on the fact that neither the server threads nor sv_temptimer are running here to be able to safely traverse those lists of sockets.

But it looks to me like that's no longer true--we're shutting down just one namespace here, and others may still be running. If so and if they modify sv_tempsocks or sv_permsocks while we're running through them then we're going to get a crash.

--b.

Quoted 5 lines

Reviewing your more recent patches I think we have a problem with some of the code that's already merged. See the comment in svc_shutdown_net:

	void svc_shutdown_net(struct svc_serv *serv, struct net *net)
	{

By the way, note there's some preexisting trouble here:

Quoted 7 lines

/*
* The set of xprts (contained in the sv_tempsocks and
* sv_permsocks lists) is now constant, since it is modified
* only by accepting new sockets (done by service threads in
* svc_recv) or aging old ones (done by sv_temptimer), or
* configuration changes (excluded by whatever locking the
* caller is using--nfsd_mutex in the case of nfsd).

I don't think the callers are as careful about this as they should be, so I think there may be some cases where we could crash if there are multiple processes concurrently trying to start, stop, and/or modify the listening sockets of a server.

We need to fix that too.

(I haven't actually seen that bug in practice. We *did* see people hit bugs on shutdown of a busy server before fixing the receive/shutdown races, though.)

--b.

Quoted 19 lines

		So it's
		 * safe to traverse those lists and shut everything down:
		 */
		svc_close_net(serv, net);
	
		if (serv->sv_shutdown)
			serv->sv_shutdown(serv, net);
	}

So we depend on the fact that neither the server threads nor sv_temptimer are running here to be able to safely traverse those lists of sockets.

But it looks to me like that's no longer true--we're shutting down just one namespace here, and others may still be running. If so and if they modify sv_tempsocks or sv_permsocks while we're running through them then we're going to get a crash.

--b.

Service sv_tempsocks and sv_permsocks lists are accessible by tasks with different network namespaces, and thus per-net service destruction must be protected.
These lists are protected by service sv_lock. So lets wrap list munipulations with this lock and move tranports destruction outside wrapped area to prevent deadlocks.

Signed-off-by Stanislav Kinsbursky <[email protected]>

---
 net/sunrpc/svc_xprt.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

net/sunrpc/svc_xprt.c +25 -4

index 8195c6a..233f993 100644
@@ -954,7 +954,8 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
 	}
 }
 
-static void svc_clear_list(struct list_head *xprt_list, struct net *net)
+static void svc_clear_list(struct list_head *xprt_list, struct net *net,
+			   struct list_head *kill_list)
 {
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;

@@ -962,7 +963,8 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
 		if (xprt->xpt_net != net)
 			continue;
-		svc_delete_xprt(xprt);
+		list_move(&xprt->xpt_list, kill_list);
+		set_bit(XPT_DETACHED, &xprt->xpt_flags);
 	}
 	list_for_each_entry(xprt, xprt_list, xpt_list)
 		BUG_ON(xprt->xpt_net == net);

@@ -970,6 +972,15 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 
 void svc_close_net(struct svc_serv *serv, struct net *net)
 {
+	struct svc_xprt *xprt;
+	LIST_HEAD(kill_list);
+
+	/*
+	 * Protect the lists, since they can be by tasks with different network
+	 * namespace contexts.
+	 */
+	spin_lock(&serv->sv_lock);
+
 	svc_close_list(&serv->sv_tempsocks, net);
 	svc_close_list(&serv->sv_permsocks, net);
 

@@ -979,8 +990,18 @@ void svc_close_net(struct svc_serv *serv, struct net *net)
 	 * svc_enqueue will not add new entries without taking the
 	 * sp_lock and checking XPT_BUSY.
 	 */
-	svc_clear_list(&serv->sv_tempsocks, net);
-	svc_clear_list(&serv->sv_permsocks, net);
+	svc_clear_list(&serv->sv_tempsocks, net, &kill_list);
+	svc_clear_list(&serv->sv_permsocks, net, &kill_list);
+
+	spin_unlock(&serv->sv_lock);
+
+	/*
+	 * Destroy collected transports.
+	 * Note: tranports has been marked as XPT_DETACHED on svc_clear_list(),
+	 * so no need to protect againt list_del() in svc_delete_xprt().
+	 */
+	list_for_each_entry(xprt, &kill_list, xpt_list)
+		svc_delete_xprt(xprt);
 }
 
 /*

Reviewing your more recent patches I think we have a problem with some of the code that's already merged. See the comment in svc_shutdown_net:

void svc_shutdown_net(struct svc_serv *serv, struct net *net) {
/*
* The set of xprts (contained in the sv_tempsocks and
* sv_permsocks lists) is now constant, since it is modified
* only by accepting new sockets (done by service threads in
* svc_recv) or aging old ones (done by sv_temptimer), or
* configuration changes (excluded by whatever locking the
* caller is using--nfsd_mutex in the case of nfsd). So it's
* safe to traverse those lists and shut everything down:
*/
svc_close_net(serv, net);

if (serv->sv_shutdown)
serv->sv_shutdown(serv, net);
}

So we depend on the fact that neither the server threads nor sv_temptimer are running here to be able to safely traverse those lists of sockets.

But it looks to me like that's no longer true--we're shutting down just one namespace here, and others may still be running. If so and if they modify sv_tempsocks or sv_permsocks while we're running through them then we're going to get a crash.