[PATCH] auth_gss: unregister gss_domain when unloading module

[PATCH] auth_gss: unregister gss_domain when unloading module

[Akinobu Mita]

Reloading rpcsec_gss_krb5 or rpcsec_gss_spkm3 hit duplicate
registration in svcauth_gss_register_pseudoflavor().
(If DEBUG_PAGEALLOC is enabled, oops will happen at
auth_domain_put() --> hlist_del() with uninitialized hlist_node)

svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name)
{
	...

        test = auth_domain_lookup(name, &new->h);
        if (test != &new->h) { /* XXX Duplicate registration? */
                auth_domain_put(&new->h);
                /* dangling ref-count... */
	...
}

This patch unregisters gss_domain and free it when unloading
modules (rpcsec_gss_krb5 or rpcsec_gss_spkm3 module call
gss_mech_unregister())

Cc: Andy Adamson <andros@citi.umich.edu>
Cc: "J. Bruce Fields" <bfields@citi.umich.edu>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 net/sunrpc/auth_gss/gss_mech_switch.c |    4 ++++
 net/sunrpc/auth_gss/svcauth_gss.c     |    6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

Index: work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
===================================================================
--- work-fault-inject.orig/net/sunrpc/auth_gss/gss_mech_switch.c
+++ work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
@@ -59,7 +59,11 @@ gss_mech_free(struct gss_api_mech *gm)
 	int i;
 
 	for (i = 0; i < gm->gm_pf_num; i++) {
+		struct auth_domain *dom;
+
 		pf = &gm->gm_pfs[i];
+		dom = auth_domain_find(pf->auth_domain_name);
+		auth_domain_put(dom);
 		kfree(pf->auth_domain_name);
 		pf->auth_domain_name = NULL;
 	}
Index: work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
===================================================================
--- work-fault-inject.orig/net/sunrpc/auth_gss/svcauth_gss.c
+++ work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
@@ -765,9 +765,9 @@ svcauth_gss_register_pseudoflavor(u32 ps
 
 	test = auth_domain_lookup(name, &new->h);
 	if (test != &new->h) { /* XXX Duplicate registration? */
-		auth_domain_put(&new->h);
-		/* dangling ref-count... */
-		goto out;
+		WARN_ON(1);
+		kfree(new->h.name);
+		goto out_free_dom;
 	}
 	return 0;
 

Re: [PATCH] auth_gss: unregister gss_domain when unloading module

[Akinobu Mita]

On Sun, Oct 29, 2006 at 03:55:54AM +0900, Akinobu Mita wrote:
> This patch unregisters gss_domain and free it when unloading
> modules (rpcsec_gss_krb5 or rpcsec_gss_spkm3 module call
> gss_mech_unregister())

This patch was wrong. Because it didn't manipulate refcount well.
And I found another problem in linux/net/sunrpc/svcauth.c, too.

[PATCH 1/2] sunrpc: add missing spin_unlock

[Akinobu Mita]

auth_domain_put() forgot to unlock acquired spinlock.

Cc: Olaf Kirch <okir@monad.swb.de>
Cc: Andy Adamson <andros@citi.umich.edu>
Cc: J. Bruce Fields <bfields@citi.umich.edu>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Index: work-fault-inject/net/sunrpc/svcauth.c
===================================================================
--- work-fault-inject.orig/net/sunrpc/svcauth.c
+++ work-fault-inject/net/sunrpc/svcauth.c
@@ -126,6 +126,7 @@ void auth_domain_put(struct auth_domain 
 	if (atomic_dec_and_lock(&dom->ref.refcount, &auth_domain_lock)) {
 		hlist_del(&dom->hash);
 		dom->flavour->domain_release(dom);
+		spin_unlock(&auth_domain_lock);
 	}
 }
 

[PATCH 2/2] auth_gss: unregister gss_domain when unloading module

[Akinobu Mita]

Reloading rpcsec_gss_krb5 or rpcsec_gss_spkm3 hit duplicate
registration in svcauth_gss_register_pseudoflavor().
(If DEBUG_PAGEALLOC is enabled, oops will happen at
auth_domain_put() --> hlist_del() with uninitialized hlist_node)

svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name)
{
	...

        test = auth_domain_lookup(name, &new->h);
        if (test != &new->h) { /* XXX Duplicate registration? */
                auth_domain_put(&new->h);
                /* dangling ref-count... */
	...
}

This patch unregisters gss_domain and free it when unloading
modules.

- Define svcauth_gss_unregister_pseudoflavor()
  (doing the opposite of svcauth_gss_register_pseudoflavor())

- Call svcauth_gss_unregister_pseudoflavor() in gss_mech_free()

Cc: Andy Adamson <andros@citi.umich.edu>
Cc: J. Bruce Fields <bfields@citi.umich.edu>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 net/sunrpc/auth_gss/gss_mech_switch.c |    4 ++++
 net/sunrpc/auth_gss/svcauth_gss.c     |    6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

Index: work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
===================================================================
--- work-fault-inject.orig/net/sunrpc/auth_gss/gss_mech_switch.c
+++ work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
@@ -60,6 +60,9 @@ gss_mech_free(struct gss_api_mech *gm)
 
 	for (i = 0; i < gm->gm_pf_num; i++) {
 		pf = &gm->gm_pfs[i];
+		if (!pf->auth_domain_name)
+			continue;
+		svcauth_gss_unregister_pseudoflavor(pf->auth_domain_name);
 		kfree(pf->auth_domain_name);
 		pf->auth_domain_name = NULL;
 	}
@@ -93,8 +96,11 @@ gss_mech_svc_setup(struct gss_api_mech *
 			goto out;
 		status = svcauth_gss_register_pseudoflavor(pf->pseudoflavor,
 							pf->auth_domain_name);
-		if (status)
+		if (status) {
+			kfree(pf->auth_domain_name);
+			pf->auth_domain_name = NULL;
 			goto out;
+		}
 	}
 	return 0;
 out:
Index: work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
===================================================================
--- work-fault-inject.orig/net/sunrpc/auth_gss/svcauth_gss.c
+++ work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
@@ -765,10 +765,12 @@ svcauth_gss_register_pseudoflavor(u32 ps
 
 	test = auth_domain_lookup(name, &new->h);
 	if (test != &new->h) { /* XXX Duplicate registration? */
-		auth_domain_put(&new->h);
-		/* dangling ref-count... */
-		goto out;
+		WARN_ON(1);
+		stat = -EBUSY;
+		kfree(new->h.name);
+		goto out_free_dom;
 	}
+	auth_domain_put(&new->h);
 	return 0;
 
 out_free_dom:
@@ -779,6 +781,19 @@ out:
 
 EXPORT_SYMBOL(svcauth_gss_register_pseudoflavor);
 
+void svcauth_gss_unregister_pseudoflavor(char *name)
+{
+	struct auth_domain *dom;
+
+	dom = auth_domain_find(name);
+	if (dom) {
+		auth_domain_put(dom);
+		auth_domain_put(dom);
+	}
+}
+
+EXPORT_SYMBOL(svcauth_gss_unregister_pseudoflavor);
+
 static inline int
 read_u32_from_xdr_buf(struct xdr_buf *buf, int base, u32 *obj)
 {
Index: work-fault-inject/include/linux/sunrpc/svcauth_gss.h
===================================================================
--- work-fault-inject.orig/include/linux/sunrpc/svcauth_gss.h
+++ work-fault-inject/include/linux/sunrpc/svcauth_gss.h
@@ -22,6 +22,7 @@
 int gss_svc_init(void);
 void gss_svc_shutdown(void);
 int svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name);
+void svcauth_gss_unregister_pseudoflavor(char *name);
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_SUNRPC_SVCAUTH_GSS_H */

Re: [PATCH] auth_gss: unregister gss_domain when unloading module

[Trond Myklebust]

On Sun, 2006-10-29 at 03:55 +0900, Akinobu Mita wrote:
> Reloading rpcsec_gss_krb5 or rpcsec_gss_spkm3 hit duplicate
> registration in svcauth_gss_register_pseudoflavor().
> (If DEBUG_PAGEALLOC is enabled, oops will happen at
> auth_domain_put() --> hlist_del() with uninitialized hlist_node)
> 
> svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name)
> {
> 	...
> 
>         test = auth_domain_lookup(name, &new->h);
>         if (test != &new->h) { /* XXX Duplicate registration? */
>                 auth_domain_put(&new->h);
>                 /* dangling ref-count... */
> 	...
> }
> 
> This patch unregisters gss_domain and free it when unloading
> modules (rpcsec_gss_krb5 or rpcsec_gss_spkm3 module call
> gss_mech_unregister())
> 
> Cc: Andy Adamson <andros@citi.umich.edu>
> Cc: "J. Bruce Fields" <bfields@citi.umich.edu>
> Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> 
>  net/sunrpc/auth_gss/gss_mech_switch.c |    4 ++++
>  net/sunrpc/auth_gss/svcauth_gss.c     |    6 +++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> Index: work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
> ===================================================================
> --- work-fault-inject.orig/net/sunrpc/auth_gss/gss_mech_switch.c
> +++ work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
> @@ -59,7 +59,11 @@ gss_mech_free(struct gss_api_mech *gm)
>  	int i;
>  
>  	for (i = 0; i < gm->gm_pf_num; i++) {
> +		struct auth_domain *dom;
> +
>  		pf = &gm->gm_pfs[i];
> +		dom = auth_domain_find(pf->auth_domain_name);
> +		auth_domain_put(dom);

Since auth_domain_find() takes a reference on "dom", and
auth_domain_put() releases it, won't this just be a no-op?

>  		kfree(pf->auth_domain_name);
>  		pf->auth_domain_name = NULL;
>  	}
> Index: work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
> ===================================================================
> --- work-fault-inject.orig/net/sunrpc/auth_gss/svcauth_gss.c
> +++ work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -765,9 +765,9 @@ svcauth_gss_register_pseudoflavor(u32 ps
>  
>  	test = auth_domain_lookup(name, &new->h);
>  	if (test != &new->h) { /* XXX Duplicate registration? */
> -		auth_domain_put(&new->h);
> -		/* dangling ref-count... */
> -		goto out;
> +		WARN_ON(1);
> +		kfree(new->h.name);
> +		goto out_free_dom;
>  	}
>  	return 0;
>  

Cheers,
  Trond

Re: [PATCH 2/2] auth_gss: unregister gss_domain when unloading module

[Trond Myklebust]

On Sun, 2006-10-29 at 22:38 +0900, Akinobu Mita wrote:
> Reloading rpcsec_gss_krb5 or rpcsec_gss_spkm3 hit duplicate
> registration in svcauth_gss_register_pseudoflavor().
> (If DEBUG_PAGEALLOC is enabled, oops will happen at
> auth_domain_put() --> hlist_del() with uninitialized hlist_node)
> 
> svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name)
> {
> 	...
> 
>         test = auth_domain_lookup(name, &new->h);
>         if (test != &new->h) { /* XXX Duplicate registration? */
>                 auth_domain_put(&new->h);
>                 /* dangling ref-count... */
> 	...
> }
> 
> This patch unregisters gss_domain and free it when unloading
> modules.
> 
> - Define svcauth_gss_unregister_pseudoflavor()
>   (doing the opposite of svcauth_gss_register_pseudoflavor())
> 
> - Call svcauth_gss_unregister_pseudoflavor() in gss_mech_free()
> 
> Cc: Andy Adamson <andros@citi.umich.edu>
> Cc: J. Bruce Fields <bfields@citi.umich.edu>
> Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> 
>  net/sunrpc/auth_gss/gss_mech_switch.c |    4 ++++
>  net/sunrpc/auth_gss/svcauth_gss.c     |    6 +++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> Index: work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
> ===================================================================
> --- work-fault-inject.orig/net/sunrpc/auth_gss/gss_mech_switch.c
> +++ work-fault-inject/net/sunrpc/auth_gss/gss_mech_switch.c
> @@ -60,6 +60,9 @@ gss_mech_free(struct gss_api_mech *gm)
>  
>  	for (i = 0; i < gm->gm_pf_num; i++) {
>  		pf = &gm->gm_pfs[i];
> +		if (!pf->auth_domain_name)
> +			continue;
> +		svcauth_gss_unregister_pseudoflavor(pf->auth_domain_name);
>  		kfree(pf->auth_domain_name);
>  		pf->auth_domain_name = NULL;
>  	}
> @@ -93,8 +96,11 @@ gss_mech_svc_setup(struct gss_api_mech *
>  			goto out;
>  		status = svcauth_gss_register_pseudoflavor(pf->pseudoflavor,
>  							pf->auth_domain_name);
> -		if (status)
> +		if (status) {
> +			kfree(pf->auth_domain_name);
> +			pf->auth_domain_name = NULL;
>  			goto out;
> +		}
>  	}
>  	return 0;
>  out:
> Index: work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
> ===================================================================
> --- work-fault-inject.orig/net/sunrpc/auth_gss/svcauth_gss.c
> +++ work-fault-inject/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -765,10 +765,12 @@ svcauth_gss_register_pseudoflavor(u32 ps
>  
>  	test = auth_domain_lookup(name, &new->h);
>  	if (test != &new->h) { /* XXX Duplicate registration? */
> -		auth_domain_put(&new->h);
> -		/* dangling ref-count... */
> -		goto out;
> +		WARN_ON(1);
> +		stat = -EBUSY;
> +		kfree(new->h.name);
> +		goto out_free_dom;
>  	}
> +	auth_domain_put(&new->h);
>  	return 0;
>  
>  out_free_dom:
> @@ -779,6 +781,19 @@ out:
>  
>  EXPORT_SYMBOL(svcauth_gss_register_pseudoflavor);
>  
> +void svcauth_gss_unregister_pseudoflavor(char *name)
> +{
> +	struct auth_domain *dom;
> +
> +	dom = auth_domain_find(name);
> +	if (dom) {
> +		auth_domain_put(dom);
> +		auth_domain_put(dom);
> +	}
> +}

Strictly speaking, if you want to be smp-safe, you probably need
something like the following:

	dom = auth_domain_find(name);
	if (dom) {
		spin_lock(&auth_domain_lock);
		if (!hlist_unhashed(dom->hash)) {
			hlist_del_init(dom->hash);
			spin_unlock(&auth_domain_lock);
			auth_domain_put(dom);
		} else
			spin_unlock(&auth_domain_lock);
		auth_domain_put(dom);
	}

and then add a test for hlist_unhashed into auth_domain_put(). If not,
some other processor could race you inside
svcauth_gss_unregister_pseudoflavor.

However since all this is being done under the BKL, then your
alternative above might be sufficient. The only question then would be
why we need auth_domain_lock?

> +EXPORT_SYMBOL(svcauth_gss_unregister_pseudoflavor);
> +
>  static inline int
>  read_u32_from_xdr_buf(struct xdr_buf *buf, int base, u32 *obj)
>  {
> Index: work-fault-inject/include/linux/sunrpc/svcauth_gss.h
> ===================================================================
> --- work-fault-inject.orig/include/linux/sunrpc/svcauth_gss.h
> +++ work-fault-inject/include/linux/sunrpc/svcauth_gss.h
> @@ -22,6 +22,7 @@
>  int gss_svc_init(void);
>  void gss_svc_shutdown(void);
>  int svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name);
> +void svcauth_gss_unregister_pseudoflavor(char *name);
>  
>  #endif /* __KERNEL__ */
>  #endif /* _LINUX_SUNRPC_SVCAUTH_GSS_H */

Cheers,
  Trond

Re: [PATCH 1/2] sunrpc: add missing spin_unlock

[Trond Myklebust]

On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> auth_domain_put() forgot to unlock acquired spinlock.

ACK. (and added Neil to the CC list).

Cheers,
  Trond

> 
> Cc: Olaf Kirch <okir@monad.swb.de>
> Cc: Andy Adamson <andros@citi.umich.edu>
> Cc: J. Bruce Fields <bfields@citi.umich.edu>
> Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> 
> Index: work-fault-inject/net/sunrpc/svcauth.c
> ===================================================================
> --- work-fault-inject.orig/net/sunrpc/svcauth.c
> +++ work-fault-inject/net/sunrpc/svcauth.c
> @@ -126,6 +126,7 @@ void auth_domain_put(struct auth_domain 
>  	if (atomic_dec_and_lock(&dom->ref.refcount, &auth_domain_lock)) {
>  		hlist_del(&dom->hash);
>  		dom->flavour->domain_release(dom);
> +		spin_unlock(&auth_domain_lock);
>  	}
>  }
>  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] sunrpc: add missing spin_unlock

[Neil Brown]

On Sunday October 29, Trond.Myklebust@netapp.com wrote:
> On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> > auth_domain_put() forgot to unlock acquired spinlock.
> 
> ACK. (and added Neil to the CC list).

Thanks.  Also ACK.

However this raises the question of why no-one has reported spinlock
deadlocks despite this bug being in 2.6.17 and 2.6.18.
It turns out the code was never exercised due to other bugs.

This patch fixes those.  Andrew: I notice the first fix has gone into
rc3-mm1. Thanks.  If that and this could get to rc4, that would be
great.

Thanks everyone,
NeilBrown

------------------------
Subject: Fix refcounting problems in rpc servers

From: Neil Brown <neilb@suse.de>

A recent patch fixed a problem which would occur when the refcount on
an auth_domain reached zero.  This problem has not been reported in
practice despite existing in two major kernel releases because the
refcount can never reach zero.

This patch fixes the problems that stop the refcount reaching zero.

1/ We were adding to the refcount when inserting in the hash table,
   but only removing from the hashtable when the refcount reached zero.
   Obviously it never would.  So don't count the implied reference of
   being in the hash table.

2/ There are two paths on which a socket can be destroyed.  One called
   svcauth_unix_info_release().  The other didn't.  So when the other was
   taken, we can lose a reference to an ip_map which in-turn holds a
   reference to an auth_domain

   So unify the exit paths into svc_sock_put.  This highlights the fact
   that svc_delete_socket has slightly odd semantics - it does not drop
   a reference but probably should.  Fixing this need a bit more
   thought and testing.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./net/sunrpc/svcauth.c |    4 +---
 ./net/sunrpc/svcsock.c |   30 ++++++++++++++----------------
 2 files changed, 15 insertions(+), 19 deletions(-)

diff .prev/net/sunrpc/svcauth.c ./net/sunrpc/svcauth.c
--- .prev/net/sunrpc/svcauth.c	2006-10-30 15:41:10.000000000 +1100
+++ ./net/sunrpc/svcauth.c	2006-10-30 16:12:00.000000000 +1100
@@ -148,10 +148,8 @@ auth_domain_lookup(char *name, struct au
 			return hp;
 		}
 	}
-	if (new) {
+	if (new)
 		hlist_add_head(&new->hash, head);
-		kref_get(&new->ref);
-	}
 	spin_unlock(&auth_domain_lock);
 	return new;
 }

diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
--- .prev/net/sunrpc/svcsock.c	2006-10-30 15:26:54.000000000 +1100
+++ ./net/sunrpc/svcsock.c	2006-10-30 16:22:58.000000000 +1100
@@ -330,8 +330,13 @@ static inline void
 svc_sock_put(struct svc_sock *svsk)
 {
 	if (atomic_dec_and_test(&svsk->sk_inuse) && test_bit(SK_DEAD, &svsk->sk_flags)) {
-		dprintk("svc: releasing dead socket\n");
-		sock_release(svsk->sk_sock);
+		printk("svc: releasing dead socket\n");
+		if (svsk->sk_sock->file)
+			sockfd_put(svsk->sk_sock);
+		else
+			sock_release(svsk->sk_sock);
+		if (svsk->sk_info_authunix != NULL)
+			svcauth_unix_info_release(svsk->sk_info_authunix);
 		kfree(svsk);
 	}
 }
@@ -1636,20 +1641,13 @@ svc_delete_socket(struct svc_sock *svsk)
 		if (test_bit(SK_TEMP, &svsk->sk_flags))
 			serv->sv_tmpcnt--;
 
-	if (!atomic_read(&svsk->sk_inuse)) {
-		spin_unlock_bh(&serv->sv_lock);
-		if (svsk->sk_sock->file)
-			sockfd_put(svsk->sk_sock);
-		else
-			sock_release(svsk->sk_sock);
-		if (svsk->sk_info_authunix != NULL)
-			svcauth_unix_info_release(svsk->sk_info_authunix);
-		kfree(svsk);
-	} else {
-		spin_unlock_bh(&serv->sv_lock);
-		dprintk(KERN_NOTICE "svc: server socket destroy delayed\n");
-		/* svsk->sk_server = NULL; */
-	}
+	/* This atomic_inc should be needed - svc_delete_socket
+	 * should have the semantic of dropping a reference.
+	 * But it doesn't yet....
+	 */
+	atomic_inc(&svsk->sk_inuse);
+	spin_unlock_bh(&serv->sv_lock);
+	svc_sock_put(svsk);
 }
 
 /*

Re: [PATCH 2/2] auth_gss: unregister gss_domain when unloading module

[Akinobu Mita]

On Sun, Oct 29, 2006 at 03:15:59PM -0500, Trond Myklebust wrote:

> > +void svcauth_gss_unregister_pseudoflavor(char *name)
> > +{
> > +	struct auth_domain *dom;
> > +
> > +	dom = auth_domain_find(name);
> > +	if (dom) {
> > +		auth_domain_put(dom);
> > +		auth_domain_put(dom);
> > +	}
> > +}
> 
> Strictly speaking, if you want to be smp-safe, you probably need
> something like the following:
> 
> 	dom = auth_domain_find(name);
> 	if (dom) {
> 		spin_lock(&auth_domain_lock);
> 		if (!hlist_unhashed(dom->hash)) {
> 			hlist_del_init(dom->hash);
> 			spin_unlock(&auth_domain_lock);
> 			auth_domain_put(dom);
> 		} else
> 			spin_unlock(&auth_domain_lock);
> 		auth_domain_put(dom);
> 	}
> 
> and then add a test for hlist_unhashed into auth_domain_put(). If not,
> some other processor could race you inside
> svcauth_gss_unregister_pseudoflavor.

But auth_domain_table is protected by auth_domain_lock while we are
using auth_domain_put()/auth_domain_lookup()/auth_domain_find().
So I think there is not big difference.

[PATCH -mm] sunrpc/auth_gss: auth_domain refcount fix

[Akinobu Mita]

On Mon, Oct 30, 2006 at 04:43:35PM +1100, Neil Brown wrote:

> 1/ We were adding to the refcount when inserting in the hash table,
>    but only removing from the hashtable when the refcount reached zero.
>    Obviously it never would.  So don't count the implied reference of
>    being in the hash table.

...

> diff .prev/net/sunrpc/svcauth.c ./net/sunrpc/svcauth.c
> --- .prev/net/sunrpc/svcauth.c	2006-10-30 15:41:10.000000000 +1100
> +++ ./net/sunrpc/svcauth.c	2006-10-30 16:12:00.000000000 +1100
> @@ -148,10 +148,8 @@ auth_domain_lookup(char *name, struct au
>  			return hp;
>  		}
>  	}
> -	if (new) {
> +	if (new)
>  		hlist_add_head(&new->hash, head);
> -		kref_get(&new->ref);
> -	}
>  	spin_unlock(&auth_domain_lock);
>  	return new;
>  }

This refcount change affects [PATCH 2/2].
(http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc3/2.6.19-rc3-mm1/broken-out/auth_gss-unregister-gss_domain-when-unloading-module.patch)

Subject: sunrpc/auth_gss: auth_domain refcount fix

auth_domain_lookup() has been changed not to increase refcount when
inserting in the hash table.

Cc: Neil Brown <neilb@suse.de>
Cc: Andy Adamson <andros@citi.umich.edu>
Cc: J. Bruce Fields <bfields@citi.umich.edu>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Index: 2.6-mm/net/sunrpc/auth_gss/svcauth_gss.c
===================================================================
--- 2.6-mm.orig/net/sunrpc/auth_gss/svcauth_gss.c
+++ 2.6-mm/net/sunrpc/auth_gss/svcauth_gss.c
@@ -770,7 +770,6 @@ svcauth_gss_register_pseudoflavor(u32 ps
 		kfree(new->h.name);
 		goto out_free_dom;
 	}
-	auth_domain_put(&new->h);
 	return 0;
 
 out_free_dom:

Re: [PATCH 2/2] auth_gss: unregister gss_domain when unloading module

[Trond Myklebust]

On Mon, 2006-10-30 at 23:54 +0900, Akinobu Mita wrote:
> On Sun, Oct 29, 2006 at 03:15:59PM -0500, Trond Myklebust wrote:
> 
> > > +void svcauth_gss_unregister_pseudoflavor(char *name)
> > > +{
> > > +	struct auth_domain *dom;
> > > +
> > > +	dom = auth_domain_find(name);
> > > +	if (dom) {
> > > +		auth_domain_put(dom);
> > > +		auth_domain_put(dom);
> > > +	}
> > > +}
> > 
> > Strictly speaking, if you want to be smp-safe, you probably need
> > something like the following:
> > 
> > 	dom = auth_domain_find(name);
> > 	if (dom) {
> > 		spin_lock(&auth_domain_lock);
> > 		if (!hlist_unhashed(dom->hash)) {
> > 			hlist_del_init(dom->hash);
> > 			spin_unlock(&auth_domain_lock);
> > 			auth_domain_put(dom);
> > 		} else
> > 			spin_unlock(&auth_domain_lock);
> > 		auth_domain_put(dom);
> > 	}
> > 
> > and then add a test for hlist_unhashed into auth_domain_put(). If not,
> > some other processor could race you inside
> > svcauth_gss_unregister_pseudoflavor.
> 
> But auth_domain_table is protected by auth_domain_lock while we are
> using auth_domain_put()/auth_domain_lookup()/auth_domain_find().
> So I think there is not big difference.

No. The auth_domain_lock was released after the call to
auth_domain_find(), and thus there is no guarantee that the entry is
still referenced when you get round to that second call to
auth_domain_put(). Testing for hlist_unhashed() and then removing the
entry from the lookup table while under the spin lock fixes this
problem: it ensures that you only call auth_domain_put() once if some
other process has raced you.

Actually, making a helper "auth_domain_find_and_unhash()" would probably
be even more efficient in this case, since that would enable you to
reduce the auth_domain_lock usage further.

Cheers,
  Trond

Re: [PATCH 2/2] auth_gss: unregister gss_domain when unloading module

[Akinobu Mita]

2006/10/31, Trond Myklebust <Trond.Myklebust@netapp.com>:
> On Mon, 2006-10-30 at 23:54 +0900, Akinobu Mita wrote:
> > On Sun, Oct 29, 2006 at 03:15:59PM -0500, Trond Myklebust wrote:
> >
> > > > +void svcauth_gss_unregister_pseudoflavor(char *name)
> > > > +{
> > > > + struct auth_domain *dom;
> > > > +
> > > > + dom = auth_domain_find(name);
> > > > + if (dom) {
> > > > +         auth_domain_put(dom);
> > > > +         auth_domain_put(dom);
> > > > + }
> > > > +}
> > >
> > > Strictly speaking, if you want to be smp-safe, you probably need
> > > something like the following:
> > >
> > >     dom = auth_domain_find(name);
> > >     if (dom) {
> > >             spin_lock(&auth_domain_lock);
> > >             if (!hlist_unhashed(dom->hash)) {
> > >                     hlist_del_init(dom->hash);
> > >                     spin_unlock(&auth_domain_lock);
> > >                     auth_domain_put(dom);
> > >             } else
> > >                     spin_unlock(&auth_domain_lock);
> > >             auth_domain_put(dom);
> > >     }
> > >
> > > and then add a test for hlist_unhashed into auth_domain_put(). If not,
> > > some other processor could race you inside
> > > svcauth_gss_unregister_pseudoflavor.
> >
> > But auth_domain_table is protected by auth_domain_lock while we are
> > using auth_domain_put()/auth_domain_lookup()/auth_domain_find().
> > So I think there is not big difference.
>
> No. The auth_domain_lock was released after the call to
> auth_domain_find(), and thus there is no guarantee that the entry is
> still referenced when you get round to that second call to
> auth_domain_put(). Testing for hlist_unhashed() and then removing the
> entry from the lookup table while under the spin lock fixes this
> problem: it ensures that you only call auth_domain_put() once if some
> other process has raced you.
>

Thanks, I understand it.

But I noticed that even if we have this kind of smp-safe code, there
is no guarantee that 2nd auth_domain_put() in
svcauth_gss_unregister_pseudoflavor() is the last reference of
this gss_domain.

So it is possible to happen invalid dereference by real last user of
this gss_domain after unloading module. If this is not wrong,
Is it neccesary to have try_get_module()/put_module() somewhere to
prevent this?

Re: [PATCH 2/2] auth_gss: unregister gss_domain when unloading module

[Neil Brown]

On Tuesday October 31, akinobu.mita@gmail.com wrote:
> 
> But I noticed that even if we have this kind of smp-safe code, there
> is no guarantee that 2nd auth_domain_put() in
> svcauth_gss_unregister_pseudoflavor() is the last reference of
> this gss_domain.
> 
> So it is possible to happen invalid dereference by real last user of
> this gss_domain after unloading module. If this is not wrong,
> Is it neccesary to have try_get_module()/put_module() somewhere to
> prevent this?

After a quick look, it seems to me that one reasonable option would
be:

  svcauth_gss_register_pseudoflavor
     returns a void* which is 'new', and possible calls try_get_module(),
     failing if that fails. and
  svcauth_gss_unregister_pseudoflavor
     takes the void* (not a name) and calls auth_domain_put on it, and
     then calls put_module().

  'struct pf_desc' would have to gain a
        void * handle;
  to hold the returned value.

Would that solve the problem as you see it?

NeilBrown

Re: [PATCH 1/2] sunrpc: add missing spin_unlock

[Peter Zijlstra]

On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> auth_domain_put() forgot to unlock acquired spinlock.
> 
> Cc: Olaf Kirch <okir@monad.swb.de>
> Cc: Andy Adamson <andros@citi.umich.edu>
> Cc: J. Bruce Fields <bfields@citi.umich.edu>
> Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

I just found this too while trying to get .19-rc4-git up and running on
a machine here - took me a few hours.

It made my kernel decidedly unhappy :-(

Andrew, could you push this and:
  http://lkml.org/lkml/2006/11/3/109
into .19 still? - those patches are needed to make todays git happy on
my machine.

> Index: work-fault-inject/net/sunrpc/svcauth.c
> ===================================================================
> --- work-fault-inject.orig/net/sunrpc/svcauth.c
> +++ work-fault-inject/net/sunrpc/svcauth.c
> @@ -126,6 +126,7 @@ void auth_domain_put(struct auth_domain 
>  	if (atomic_dec_and_lock(&dom->ref.refcount, &auth_domain_lock)) {
>  		hlist_del(&dom->hash);
>  		dom->flavour->domain_release(dom);
> +		spin_unlock(&auth_domain_lock);
>  	}
>  }

Re: [PATCH 1/2] sunrpc: add missing spin_unlock

[Andrew Morton]

On Sun, 05 Nov 2006 17:35:16 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> > auth_domain_put() forgot to unlock acquired spinlock.
> > 
> > Cc: Olaf Kirch <okir@monad.swb.de>
> > Cc: Andy Adamson <andros@citi.umich.edu>
> > Cc: J. Bruce Fields <bfields@citi.umich.edu>
> > Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> 
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> I just found this too while trying to get .19-rc4-git up and running on
> a machine here - took me a few hours.
> 
> It made my kernel decidedly unhappy :-(
> 
> Andrew, could you push this and:
>   http://lkml.org/lkml/2006/11/3/109
> into .19 still? - those patches are needed to make todays git happy on
> my machine.

OK.

> > Index: work-fault-inject/net/sunrpc/svcauth.c
> > ===================================================================
> > --- work-fault-inject.orig/net/sunrpc/svcauth.c
> > +++ work-fault-inject/net/sunrpc/svcauth.c
> > @@ -126,6 +126,7 @@ void auth_domain_put(struct auth_domain 
> >  	if (atomic_dec_and_lock(&dom->ref.refcount, &auth_domain_lock)) {
> >  		hlist_del(&dom->hash);
> >  		dom->flavour->domain_release(dom);
> > +		spin_unlock(&auth_domain_lock);
> >  	}
> >  }

I wonder if this will fix http://bugzilla.kernel.org/show_bug.cgi?id=7457

Re: [PATCH 1/2] sunrpc: add missing spin_unlock

[Peter Zijlstra]

On Sun, 2006-11-05 at 11:45 -0800, Andrew Morton wrote:
> On Sun, 05 Nov 2006 17:35:16 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> > > auth_domain_put() forgot to unlock acquired spinlock.
> > > 
> > > Cc: Olaf Kirch <okir@monad.swb.de>
> > > Cc: Andy Adamson <andros@citi.umich.edu>
> > > Cc: J. Bruce Fields <bfields@citi.umich.edu>
> > > Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > 
> > Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > 
> > I just found this too while trying to get .19-rc4-git up and running on
> > a machine here - took me a few hours.
> > 
> > It made my kernel decidedly unhappy :-(
> > 
> > Andrew, could you push this and:
> >   http://lkml.org/lkml/2006/11/3/109
> > into .19 still? - those patches are needed to make todays git happy on
> > my machine.
> 
> OK.

Thanks!

> I wonder if this will fix http://bugzilla.kernel.org/show_bug.cgi?id=7457

The scheduling while atomic part looks familiar, the rest not so.

Worth giving it a shot though...

On my machine it was the keventd workqueue that got messed up.
I have some patches that:
 - add debug_show_held_locks(current) to might_sleep() and schedule()
 - check in_atomic() and lockdep_depth after each workqueue function
   and print the last function executed
 - name some 'old_style_spin_init' locks

I'll post those patches after a cleanup...

Re: [PATCH 1/2] sunrpc: add missing spin_unlock

[Elimar Riesebieter]

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

On Sun, 05 Nov 2006 the mental interface of
Andrew Morton told:

[...]
> > > Index: work-fault-inject/net/sunrpc/svcauth.c
> > > ===================================================================
> > > --- work-fault-inject.orig/net/sunrpc/svcauth.c
> > > +++ work-fault-inject/net/sunrpc/svcauth.c
> > > @@ -126,6 +126,7 @@ void auth_domain_put(struct auth_domain 
> > >  	if (atomic_dec_and_lock(&dom->ref.refcount, &auth_domain_lock)) {
> > >  		hlist_del(&dom->hash);
> > >  		dom->flavour->domain_release(dom);
> > > +		spin_unlock(&auth_domain_lock);
> > >  	}
> > >  }
> 
> I wonder if this will fix http://bugzilla.kernel.org/show_bug.cgi?id=7457

It fixes it. Patched a native 2.6.19-rc4 and it works. Thanks.

Elimar

-- 
  Planung:
  Ersatz des Zufalls durch den Irrtum.
                                -unknown-

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]