[PATCH 1/2] svcrpc: destroy server sockets all at once

[PATCH 1/2] svcrpc: destroy server sockets all at once

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

There's no reason I can see that we need to call sv_shutdown between
closing the two lists of sockets.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 include/linux/sunrpc/svcsock.h |    2 +-
 net/sunrpc/svc.c               |    7 +------
 net/sunrpc/svc_xprt.c          |   11 ++++++++++-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
index 85c50b4..c84e974 100644
--- a/include/linux/sunrpc/svcsock.h
+++ b/include/linux/sunrpc/svcsock.h
@@ -34,7 +34,7 @@ struct svc_sock {
 /*
  * Function prototypes.
  */
-void		svc_close_all(struct list_head *);
+void		svc_close_all(struct svc_serv *);
 int		svc_recv(struct svc_rqst *, long);
 int		svc_send(struct svc_rqst *);
 void		svc_drop(struct svc_rqst *);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 6e03888..60babf0 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -528,16 +528,11 @@ svc_destroy(struct svc_serv *serv)
 
 	del_timer_sync(&serv->sv_temptimer);
 
-	svc_close_all(&serv->sv_tempsocks);
+	svc_close_all(serv);
 
 	if (serv->sv_shutdown)
 		serv->sv_shutdown(serv);
 
-	svc_close_all(&serv->sv_permsocks);
-
-	BUG_ON(!list_empty(&serv->sv_permsocks));
-	BUG_ON(!list_empty(&serv->sv_tempsocks));
-
 	cache_clean_deferred(serv);
 
 	if (svc_serv_is_pooled(serv))
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 8046c6d..099ddf9 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -929,7 +929,7 @@ void svc_close_xprt(struct svc_xprt *xprt)
 }
 EXPORT_SYMBOL_GPL(svc_close_xprt);
 
-void svc_close_all(struct list_head *xprt_list)
+static void svc_close_list(struct list_head *xprt_list)
 {
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;
@@ -947,6 +947,15 @@ void svc_close_all(struct list_head *xprt_list)
 	}
 }
 
+void svc_close_all(struct svc_serv *serv)
+{
+	svc_close_list(&serv->sv_tempsocks);
+	svc_close_list(&serv->sv_permsocks);
+	BUG_ON(!list_empty(&serv->sv_permsocks));
+	BUG_ON(!list_empty(&serv->sv_tempsocks));
+
+}
+
 /*
  * Handle defer and revisit of requests
  */
-- 
1.7.5.4

[PATCH 2/2] svcrpc: avoid memory-corruption on pool shutdown

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Socket callbacks use svc_xprt_enqueue() to add an xprt to a
pool->sp_sockets list.  In normal operation a server thread will later
come along and take the xprt off that list.  On shutdown, after all the
threads have exited, we instead manually walk the sv_tempsocks and
sv_permsocks lists to find all the xprt's and delete them.

So the sp_sockets lists don't really matter any more.  As a result,
we've mostly just ignored them and hoped they would go away.

Which has gotten us into trouble; witness for example ebc63e531cc6
"svcrpc: fix list-corrupting race on nfsd shutdown", the result of Ben
Greear noticing that a still-running svc_xprt_enqueue() could re-add an
xprt to an sp_sockets list just before it was deleted.  The fix was to
remove it from the list at the end of svc_delete_xprt().  But that only
made corruption less likely--I can see nothing that prevents a
svc_xprt_enqueue() from adding another xprt to the list at the same
moment that we're removing this xprt from the list.  In fact, despite
the earlier xpo_detach(), I don't even see what guarantees that
svc_xprt_enqueue() couldn't still be running on this xprt.

So, instead, note that svc_xprt_enqueue() essentially does:
	lock sp_lock
		if XPT_BUSY unset
			add to sp_sockets
	unlock sp_lock

So, if we do:

	set XPT_BUSY on every xprt.
	Empty every sp_sockets list, under the sp_socks locks.

Then we're left knowing that the sp_sockets lists are all empty and will
stay that way, since any svc_xprt_enqueue() will check XPT_BUSY under
the sp_lock and see it set.

And *then* we can continue deleting the xprt's.

(Thanks to Jeff Layton for being correctly suspicious of this code....)

Cc: Ben Greear <greearb@candelatech.com>
Cc: Jeff Layton <jlayton@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svc_xprt.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 099ddf9..0d80c06 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -894,14 +894,7 @@ static void svc_delete_xprt(struct svc_xprt *xprt)
 	spin_lock_bh(&serv->sv_lock);
 	if (!test_and_set_bit(XPT_DETACHED, &xprt->xpt_flags))
 		list_del_init(&xprt->xpt_list);
-	/*
-	 * The only time we're called while xpt_ready is still on a list
-	 * is while the list itself is about to be destroyed (in
-	 * svc_destroy).  BUT svc_xprt_enqueue could still be attempting
-	 * to add new entries to the sp_sockets list, so we can't leave
-	 * a freed xprt on it.
-	 */
-	list_del_init(&xprt->xpt_ready);
+	BUG_ON(!list_empty(&xprt->xpt_ready));
 	if (test_bit(XPT_TEMP, &xprt->xpt_flags))
 		serv->sv_tmpcnt--;
 	spin_unlock_bh(&serv->sv_lock);
@@ -932,28 +925,45 @@ EXPORT_SYMBOL_GPL(svc_close_xprt);
 static void svc_close_list(struct list_head *xprt_list)
 {
 	struct svc_xprt *xprt;
-	struct svc_xprt *tmp;
 
-	/*
-	 * The server is shutting down, and no more threads are running.
-	 * svc_xprt_enqueue() might still be running, but at worst it
-	 * will re-add the xprt to sp_sockets, which will soon get
-	 * freed.  So we don't bother with any more locking, and don't
-	 * leave the close to the (nonexistent) server threads:
-	 */
-	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
+	list_for_each_entry(xprt, xprt_list, xpt_list) {
 		set_bit(XPT_CLOSE, &xprt->xpt_flags);
-		svc_delete_xprt(xprt);
+		set_bit(XPT_BUSY, &xprt->xpt_flags);
 	}
 }
 
 void svc_close_all(struct svc_serv *serv)
 {
+	struct svc_pool *pool;
+	struct svc_xprt *xprt;
+	struct svc_xprt *tmp;
+	int i;
+
 	svc_close_list(&serv->sv_tempsocks);
 	svc_close_list(&serv->sv_permsocks);
+
+	for (i = 0; i < serv->sv_nrpools; i++) {
+		pool = &serv->sv_pools[i];
+
+		spin_lock_bh(&pool->sp_lock);
+		while (!list_empty(&pool->sp_sockets)) {
+			xprt = list_first_entry(&pool->sp_sockets, struct svc_xprt, xpt_ready);
+			list_del_init(&xprt->xpt_ready);
+		}
+		spin_unlock_bh(&pool->sp_lock);
+	}
+	/*
+	 * At this point the sp_sockets lists will stay empty, since
+	 * svc_enqueue will not add new entries without taking the
+	 * sp_lock and checking XPT_BUSY.
+	 */
+	list_for_each_entry_safe(xprt, tmp, &serv->sv_tempsocks, xpt_list)
+		svc_delete_xprt(xprt);
+	list_for_each_entry_safe(xprt, tmp, &serv->sv_permsocks, xpt_list)
+		svc_delete_xprt(xprt);
+
 	BUG_ON(!list_empty(&serv->sv_permsocks));
 	BUG_ON(!list_empty(&serv->sv_tempsocks));
-
 }
 
 /*
-- 
1.7.5.4

Re: [PATCH 2/2] svcrpc: avoid memory-corruption on pool shutdown

[J. Bruce Fields]

Anyone see any remaining hole, or does this finally fix the problem?

Any idea for something simpler?

If not I'll likely commit this for 3.2 and -stable before the end of the
week.

--b.

On Wed, Nov 30, 2011 at 06:40:09PM -0500, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Socket callbacks use svc_xprt_enqueue() to add an xprt to a
> pool->sp_sockets list.  In normal operation a server thread will later
> come along and take the xprt off that list.  On shutdown, after all the
> threads have exited, we instead manually walk the sv_tempsocks and
> sv_permsocks lists to find all the xprt's and delete them.
> 
> So the sp_sockets lists don't really matter any more.  As a result,
> we've mostly just ignored them and hoped they would go away.
> 
> Which has gotten us into trouble; witness for example ebc63e531cc6
> "svcrpc: fix list-corrupting race on nfsd shutdown", the result of Ben
> Greear noticing that a still-running svc_xprt_enqueue() could re-add an
> xprt to an sp_sockets list just before it was deleted.  The fix was to
> remove it from the list at the end of svc_delete_xprt().  But that only
> made corruption less likely--I can see nothing that prevents a
> svc_xprt_enqueue() from adding another xprt to the list at the same
> moment that we're removing this xprt from the list.  In fact, despite
> the earlier xpo_detach(), I don't even see what guarantees that
> svc_xprt_enqueue() couldn't still be running on this xprt.
> 
> So, instead, note that svc_xprt_enqueue() essentially does:
> 	lock sp_lock
> 		if XPT_BUSY unset
> 			add to sp_sockets
> 	unlock sp_lock
> 
> So, if we do:
> 
> 	set XPT_BUSY on every xprt.
> 	Empty every sp_sockets list, under the sp_socks locks.
> 
> Then we're left knowing that the sp_sockets lists are all empty and will
> stay that way, since any svc_xprt_enqueue() will check XPT_BUSY under
> the sp_lock and see it set.
> 
> And *then* we can continue deleting the xprt's.
> 
> (Thanks to Jeff Layton for being correctly suspicious of this code....)
> 
> Cc: Ben Greear <greearb@candelatech.com>
> Cc: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  net/sunrpc/svc_xprt.c |   48 +++++++++++++++++++++++++++++-------------------
>  1 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 099ddf9..0d80c06 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -894,14 +894,7 @@ static void svc_delete_xprt(struct svc_xprt *xprt)
>  	spin_lock_bh(&serv->sv_lock);
>  	if (!test_and_set_bit(XPT_DETACHED, &xprt->xpt_flags))
>  		list_del_init(&xprt->xpt_list);
> -	/*
> -	 * The only time we're called while xpt_ready is still on a list
> -	 * is while the list itself is about to be destroyed (in
> -	 * svc_destroy).  BUT svc_xprt_enqueue could still be attempting
> -	 * to add new entries to the sp_sockets list, so we can't leave
> -	 * a freed xprt on it.
> -	 */
> -	list_del_init(&xprt->xpt_ready);
> +	BUG_ON(!list_empty(&xprt->xpt_ready));
>  	if (test_bit(XPT_TEMP, &xprt->xpt_flags))
>  		serv->sv_tmpcnt--;
>  	spin_unlock_bh(&serv->sv_lock);
> @@ -932,28 +925,45 @@ EXPORT_SYMBOL_GPL(svc_close_xprt);
>  static void svc_close_list(struct list_head *xprt_list)
>  {
>  	struct svc_xprt *xprt;
> -	struct svc_xprt *tmp;
>  
> -	/*
> -	 * The server is shutting down, and no more threads are running.
> -	 * svc_xprt_enqueue() might still be running, but at worst it
> -	 * will re-add the xprt to sp_sockets, which will soon get
> -	 * freed.  So we don't bother with any more locking, and don't
> -	 * leave the close to the (nonexistent) server threads:
> -	 */
> -	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
> +	list_for_each_entry(xprt, xprt_list, xpt_list) {
>  		set_bit(XPT_CLOSE, &xprt->xpt_flags);
> -		svc_delete_xprt(xprt);
> +		set_bit(XPT_BUSY, &xprt->xpt_flags);
>  	}
>  }
>  
>  void svc_close_all(struct svc_serv *serv)
>  {
> +	struct svc_pool *pool;
> +	struct svc_xprt *xprt;
> +	struct svc_xprt *tmp;
> +	int i;
> +
>  	svc_close_list(&serv->sv_tempsocks);
>  	svc_close_list(&serv->sv_permsocks);
> +
> +	for (i = 0; i < serv->sv_nrpools; i++) {
> +		pool = &serv->sv_pools[i];
> +
> +		spin_lock_bh(&pool->sp_lock);
> +		while (!list_empty(&pool->sp_sockets)) {
> +			xprt = list_first_entry(&pool->sp_sockets, struct svc_xprt, xpt_ready);
> +			list_del_init(&xprt->xpt_ready);
> +		}
> +		spin_unlock_bh(&pool->sp_lock);
> +	}
> +	/*
> +	 * At this point the sp_sockets lists will stay empty, since
> +	 * svc_enqueue will not add new entries without taking the
> +	 * sp_lock and checking XPT_BUSY.
> +	 */
> +	list_for_each_entry_safe(xprt, tmp, &serv->sv_tempsocks, xpt_list)
> +		svc_delete_xprt(xprt);
> +	list_for_each_entry_safe(xprt, tmp, &serv->sv_permsocks, xpt_list)
> +		svc_delete_xprt(xprt);
> +
>  	BUG_ON(!list_empty(&serv->sv_permsocks));
>  	BUG_ON(!list_empty(&serv->sv_tempsocks));
> -
>  }
>  
>  /*
> -- 
> 1.7.5.4
> 

Re: [PATCH 2/2] svcrpc: avoid memory-corruption on pool shutdown

[Ben Greear]

On 11/30/2011 03:43 PM, J. Bruce Fields wrote:
> Anyone see any remaining hole, or does this finally fix the problem?
>
> Any idea for something simpler?
>
> If not I'll likely commit this for 3.2 and -stable before the end of the
> week.

I never did understand this code too well, so I'm afraid I can't
really review this.  I'll be sure to run our NFS stress tests when we upgrade
to newer kernels..but that will likely be a while.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 2/2] svcrpc: avoid memory-corruption on pool shutdown

[Jeff Layton]

On Wed, 30 Nov 2011 18:40:09 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Socket callbacks use svc_xprt_enqueue() to add an xprt to a
> pool->sp_sockets list.  In normal operation a server thread will later
> come along and take the xprt off that list.  On shutdown, after all the
> threads have exited, we instead manually walk the sv_tempsocks and
> sv_permsocks lists to find all the xprt's and delete them.
> 
> So the sp_sockets lists don't really matter any more.  As a result,
> we've mostly just ignored them and hoped they would go away.
> 
> Which has gotten us into trouble; witness for example ebc63e531cc6
> "svcrpc: fix list-corrupting race on nfsd shutdown", the result of Ben
> Greear noticing that a still-running svc_xprt_enqueue() could re-add an
> xprt to an sp_sockets list just before it was deleted.  The fix was to
> remove it from the list at the end of svc_delete_xprt().  But that only
> made corruption less likely--I can see nothing that prevents a
> svc_xprt_enqueue() from adding another xprt to the list at the same
> moment that we're removing this xprt from the list.  In fact, despite
> the earlier xpo_detach(), I don't even see what guarantees that
> svc_xprt_enqueue() couldn't still be running on this xprt.
> 
> So, instead, note that svc_xprt_enqueue() essentially does:
> 	lock sp_lock
> 		if XPT_BUSY unset
> 			add to sp_sockets
> 	unlock sp_lock
> 
> So, if we do:
> 
> 	set XPT_BUSY on every xprt.
> 	Empty every sp_sockets list, under the sp_socks locks.
> 
> Then we're left knowing that the sp_sockets lists are all empty and will
> stay that way, since any svc_xprt_enqueue() will check XPT_BUSY under
> the sp_lock and see it set.
> 
> And *then* we can continue deleting the xprt's.
> 
> (Thanks to Jeff Layton for being correctly suspicious of this code....)
> 
> Cc: Ben Greear <greearb@candelatech.com>
> Cc: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  net/sunrpc/svc_xprt.c |   48 +++++++++++++++++++++++++++++-------------------
>  1 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 099ddf9..0d80c06 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -894,14 +894,7 @@ static void svc_delete_xprt(struct svc_xprt *xprt)
>  	spin_lock_bh(&serv->sv_lock);
>  	if (!test_and_set_bit(XPT_DETACHED, &xprt->xpt_flags))
>  		list_del_init(&xprt->xpt_list);
> -	/*
> -	 * The only time we're called while xpt_ready is still on a list
> -	 * is while the list itself is about to be destroyed (in
> -	 * svc_destroy).  BUT svc_xprt_enqueue could still be attempting
> -	 * to add new entries to the sp_sockets list, so we can't leave
> -	 * a freed xprt on it.
> -	 */
> -	list_del_init(&xprt->xpt_ready);
> +	BUG_ON(!list_empty(&xprt->xpt_ready));
>  	if (test_bit(XPT_TEMP, &xprt->xpt_flags))
>  		serv->sv_tmpcnt--;
>  	spin_unlock_bh(&serv->sv_lock);
> @@ -932,28 +925,45 @@ EXPORT_SYMBOL_GPL(svc_close_xprt);
>  static void svc_close_list(struct list_head *xprt_list)
>  {
>  	struct svc_xprt *xprt;
> -	struct svc_xprt *tmp;
>  
> -	/*
> -	 * The server is shutting down, and no more threads are running.
> -	 * svc_xprt_enqueue() might still be running, but at worst it
> -	 * will re-add the xprt to sp_sockets, which will soon get
> -	 * freed.  So we don't bother with any more locking, and don't
> -	 * leave the close to the (nonexistent) server threads:
> -	 */
> -	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
> +	list_for_each_entry(xprt, xprt_list, xpt_list) {
>  		set_bit(XPT_CLOSE, &xprt->xpt_flags);
> -		svc_delete_xprt(xprt);
> +		set_bit(XPT_BUSY, &xprt->xpt_flags);
>  	}
>  }
>  
>  void svc_close_all(struct svc_serv *serv)
>  {
> +	struct svc_pool *pool;
> +	struct svc_xprt *xprt;
> +	struct svc_xprt *tmp;
> +	int i;
> +
>  	svc_close_list(&serv->sv_tempsocks);
>  	svc_close_list(&serv->sv_permsocks);
> +
> +	for (i = 0; i < serv->sv_nrpools; i++) {
> +		pool = &serv->sv_pools[i];
> +
> +		spin_lock_bh(&pool->sp_lock);
> +		while (!list_empty(&pool->sp_sockets)) {
> +			xprt = list_first_entry(&pool->sp_sockets, struct svc_xprt, xpt_ready);
> +			list_del_init(&xprt->xpt_ready);
> +		}
> +		spin_unlock_bh(&pool->sp_lock);
> +	}
> +	/*
> +	 * At this point the sp_sockets lists will stay empty, since
> +	 * svc_enqueue will not add new entries without taking the
> +	 * sp_lock and checking XPT_BUSY.
> +	 */
> +	list_for_each_entry_safe(xprt, tmp, &serv->sv_tempsocks, xpt_list)
> +		svc_delete_xprt(xprt);
> +	list_for_each_entry_safe(xprt, tmp, &serv->sv_permsocks, xpt_list)
> +		svc_delete_xprt(xprt);
> +

I'm always a little suspicious when I see walking and manipulating a
list outside of any locking. Can you convince me as to why it's safe to
do the above?

list_for_each_entry_safe just makes it safe to remove "xprt" from the
list essentially by preloading "tmp". What prevents another task from
removing "tmp" while you're busy dealing with "xprt"? Or from putting a
new socket onto the list at that point?

>  	BUG_ON(!list_empty(&serv->sv_permsocks));
>  	BUG_ON(!list_empty(&serv->sv_tempsocks));
> -
>  }
>  
>  /*


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] svcrpc: avoid memory-corruption on pool shutdown

[J. Bruce Fields]

On Thu, Dec 01, 2011 at 07:20:24AM -0500, Jeff Layton wrote:
> On Wed, 30 Nov 2011 18:40:09 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> >  void svc_close_all(struct svc_serv *serv)
> >  {
> > +	struct svc_pool *pool;
> > +	struct svc_xprt *xprt;
> > +	struct svc_xprt *tmp;
> > +	int i;
> > +
> >  	svc_close_list(&serv->sv_tempsocks);
> >  	svc_close_list(&serv->sv_permsocks);
> > +
> > +	for (i = 0; i < serv->sv_nrpools; i++) {
> > +		pool = &serv->sv_pools[i];
> > +
> > +		spin_lock_bh(&pool->sp_lock);
> > +		while (!list_empty(&pool->sp_sockets)) {
> > +			xprt = list_first_entry(&pool->sp_sockets, struct svc_xprt, xpt_ready);
> > +			list_del_init(&xprt->xpt_ready);
> > +		}
> > +		spin_unlock_bh(&pool->sp_lock);
> > +	}
> > +	/*
> > +	 * At this point the sp_sockets lists will stay empty, since
> > +	 * svc_enqueue will not add new entries without taking the
> > +	 * sp_lock and checking XPT_BUSY.
> > +	 */
> > +	list_for_each_entry_safe(xprt, tmp, &serv->sv_tempsocks, xpt_list)
> > +		svc_delete_xprt(xprt);
> > +	list_for_each_entry_safe(xprt, tmp, &serv->sv_permsocks, xpt_list)
> > +		svc_delete_xprt(xprt);
> > +
> 
> I'm always a little suspicious when I see walking and manipulating a
> list outside of any locking.

With good reason.

> Can you convince me as to why it's safe to do the above?

After entirely too much grepping....

The server keeps its listeners on sv_permsocks, and individual client
connections on sv_tempsocks.

The tempsocks list:
	- has a new xprt added when we accept a new connection.  This is
	  done by a server thread in svc_recv().  (Also
	  svc_check_conn_limits() may decide to remove an old connection
	  at the same time.)
	- has xprts removed by svc_age_temp_xprts, called from
	  sv_temptimer.

But svc_close_all() is called only after all server threads are gone
and after sv_temptimer has been stopped by a del_timer_sync().  So we're
safe from either of the above.

Great!  But:

The more irritating code is the code that's run on server startup and
configuration: svc_sock_update_bufs, nfsd_init_socks, svc_find_xprt,
svc_xprt_names, svc_create_xprt, svc_sock_names, and svc_addsock all
modify or traverse these two lists.  I believe the callers should all be
holding a mutex (normally nfsd_mutex, or nlmsvc_mutex or
nfs_callback_mutex as appropriate).  But not all of them are (e.g.
addfd, addxprt are fishy).

So, anyway: my inclination for now is to update this patch with a few
comments but leave it otherwise the same.

The remaining races look easier to avoid, as they'd only happen if
userspace was doing something odd like trying to give nfsd another
socket to listen on from one process while simultaneously shutting it
down from another.

But server startup and shutdown has always been fuzzy and kinda fragile.
And it will probably need a harder look anyway for containerization.  So
I'll take a harder look at that next....

--b.

> 
> list_for_each_entry_safe just makes it safe to remove "xprt" from the
> list essentially by preloading "tmp". What prevents another task from
> removing "tmp" while you're busy dealing with "xprt"? Or from putting a
> new socket onto the list at that point?
> 
> >  	BUG_ON(!list_empty(&serv->sv_permsocks));
> >  	BUG_ON(!list_empty(&serv->sv_tempsocks));
> > -
> >  }
> >  
> >  /*
> 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] svcrpc: avoid memory-corruption on pool shutdown

[J. Bruce Fields]

On Thu, Dec 01, 2011 at 05:46:40PM -0500, J. Bruce Fields wrote:
> On Thu, Dec 01, 2011 at 07:20:24AM -0500, Jeff Layton wrote:
> > On Wed, 30 Nov 2011 18:40:09 -0500
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > >  void svc_close_all(struct svc_serv *serv)
> > >  {
> > > +	struct svc_pool *pool;
> > > +	struct svc_xprt *xprt;
> > > +	struct svc_xprt *tmp;
> > > +	int i;
> > > +
> > >  	svc_close_list(&serv->sv_tempsocks);
> > >  	svc_close_list(&serv->sv_permsocks);
> > > +
> > > +	for (i = 0; i < serv->sv_nrpools; i++) {
> > > +		pool = &serv->sv_pools[i];
> > > +
> > > +		spin_lock_bh(&pool->sp_lock);
> > > +		while (!list_empty(&pool->sp_sockets)) {
> > > +			xprt = list_first_entry(&pool->sp_sockets, struct svc_xprt, xpt_ready);
> > > +			list_del_init(&xprt->xpt_ready);
> > > +		}
> > > +		spin_unlock_bh(&pool->sp_lock);
> > > +	}
> > > +	/*
> > > +	 * At this point the sp_sockets lists will stay empty, since
> > > +	 * svc_enqueue will not add new entries without taking the
> > > +	 * sp_lock and checking XPT_BUSY.
> > > +	 */
> > > +	list_for_each_entry_safe(xprt, tmp, &serv->sv_tempsocks, xpt_list)
> > > +		svc_delete_xprt(xprt);
> > > +	list_for_each_entry_safe(xprt, tmp, &serv->sv_permsocks, xpt_list)
> > > +		svc_delete_xprt(xprt);
> > > +
> > 
> > I'm always a little suspicious when I see walking and manipulating a
> > list outside of any locking.
> 
> With good reason.

(Though on the other hand you might also want to be suspicious of code
that took locks around these list traversals, as you'd have to wonder
what would gaurantee the lists would stay empty once the lock was
dropped.)

> So, anyway: my inclination for now is to update this patch with a few
> comments but leave it otherwise the same.

 	del_timer_sync(&serv->sv_temptimer);
-
+	/*
+	 * 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_all(serv);

--b.