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 <[email protected]>
Cc: "J. Bruce Fields" <[email protected]>
Cc: Trond Myklebust <[email protected]>
Signed-off-by: Akinobu Mita <[email protected]>
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;
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.
auth_domain_put() forgot to unlock acquired spinlock.
Cc: Olaf Kirch <[email protected]>
Cc: Andy Adamson <[email protected]>
Cc: J. Bruce Fields <[email protected]>
Cc: Trond Myklebust <[email protected]>
Signed-off-by: Akinobu Mita <[email protected]>
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);
}
}
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 <[email protected]>
Cc: J. Bruce Fields <[email protected]>
Cc: Trond Myklebust <[email protected]>
Signed-off-by: Akinobu Mita <[email protected]>
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 */
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 <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: Trond Myklebust <[email protected]>
> Signed-off-by: Akinobu Mita <[email protected]>
>
> 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
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 <[email protected]>
> Cc: J. Bruce Fields <[email protected]>
> Cc: Trond Myklebust <[email protected]>
> Signed-off-by: Akinobu Mita <[email protected]>
>
> 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
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 <[email protected]>
> Cc: Andy Adamson <[email protected]>
> Cc: J. Bruce Fields <[email protected]>
> Cc: Trond Myklebust <[email protected]>
> Signed-off-by: Akinobu Mita <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Sunday October 29, [email protected] 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 <[email protected]>
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 <[email protected]>
### 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);
}
/*
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.
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 <[email protected]>
Cc: Andy Adamson <[email protected]>
Cc: J. Bruce Fields <[email protected]>
Cc: Trond Myklebust <[email protected]>
Signed-off-by: Akinobu Mita <[email protected]>
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:
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
2006/10/31, Trond Myklebust <[email protected]>:
> 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?
On Tuesday October 31, [email protected] 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
On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> auth_domain_put() forgot to unlock acquired spinlock.
>
> Cc: Olaf Kirch <[email protected]>
> Cc: Andy Adamson <[email protected]>
> Cc: J. Bruce Fields <[email protected]>
> Cc: Trond Myklebust <[email protected]>
> Signed-off-by: Akinobu Mita <[email protected]>
Acked-by: Peter Zijlstra <[email protected]>
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);
> }
> }
On Sun, 05 Nov 2006 17:35:16 +0100
Peter Zijlstra <[email protected]> wrote:
> On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> > auth_domain_put() forgot to unlock acquired spinlock.
> >
> > Cc: Olaf Kirch <[email protected]>
> > Cc: Andy Adamson <[email protected]>
> > Cc: J. Bruce Fields <[email protected]>
> > Cc: Trond Myklebust <[email protected]>
> > Signed-off-by: Akinobu Mita <[email protected]>
>
> Acked-by: Peter Zijlstra <[email protected]>
>
> 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
On Sun, 2006-11-05 at 11:45 -0800, Andrew Morton wrote:
> On Sun, 05 Nov 2006 17:35:16 +0100
> Peter Zijlstra <[email protected]> wrote:
>
> > On Sun, 2006-10-29 at 22:37 +0900, Akinobu Mita wrote:
> > > auth_domain_put() forgot to unlock acquired spinlock.
> > >
> > > Cc: Olaf Kirch <[email protected]>
> > > Cc: Andy Adamson <[email protected]>
> > > Cc: J. Bruce Fields <[email protected]>
> > > Cc: Trond Myklebust <[email protected]>
> > > Signed-off-by: Akinobu Mita <[email protected]>
> >
> > Acked-by: Peter Zijlstra <[email protected]>
> >
> > 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...
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-