[PATCH] sunrpc: fix refcounting problems in rpc servers

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>
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
index 8f2320aded5c42f645b406cf51539d10228ef055..0004c1f0ef047377d1db5a645d5e9d0e745ed069 100644 (file)
--- a/net/sunrpc/svcauth.c
+++ b/net/sunrpc/svcauth.c
@@ -147,10 +147,8 @@ auth_domain_lookup(char *name, struct auth_domain *new)
                        return hp;
                }
        }
-       if (new) {
+       if (new)
                hlist_add_head(&new->hash, head);
-               kref_get(&new->ref);
-       }
        spin_unlock(&auth_domain_lock);
        return new;
 }

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 96521f16342b63e80f18f77888fa7b5b06dbd333..db0d1048d466c7cb5fccd01e7e31d61439d53096 100644 (file)
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -300,8 +300,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);
        }
 }
@@ -1604,20 +1609,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);
 }
 
 /*