SUNRPC: discard sv_refcnt, and svc_get/svc_put

sv_refcnt is no longer useful.
lockd and nfs-cb only ever have the svc active when there are a non-zero
number of threads, so sv_refcnt mirrors sv_nrthreads.

nfsd also keeps the svc active between when a socket is added and when
the first thread is started, but we don't really need a refcount for
that.  We can simply not destroy the svc while there are any permanent
sockets attached.

So remove sv_refcnt and the get/put functions.
Instead of a final call to svc_put(), call svc_destroy() instead.
This is changed to also store NULL in the passed-in pointer to make it
easier to avoid use-after-free situations.

Signed-off-by: NeilBrown <neilb@suse.de>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 81be07c1d3d156abe7f7ff1d5802601d7c5dfa38..0d6cb3fdc0e16eef619d6d5f78de2acd93b18926 100644 (file)
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -345,10 +345,10 @@ static int lockd_get(void)
 
        serv->sv_maxconn = nlm_max_connections;
        error = svc_set_num_threads(serv, NULL, 1);
-       /* The thread now holds the only reference */
-       svc_put(serv);
-       if (error < 0)
+       if (error < 0) {
+               svc_destroy(&serv);
                return error;
+       }
 
        nlmsvc_serv = serv;
        register_inetaddr_notifier(&lockd_inetaddr_notifier);
@@ -372,11 +372,9 @@ static void lockd_put(void)
        unregister_inet6addr_notifier(&lockd_inet6addr_notifier);
 #endif
 
-       svc_get(nlmsvc_serv);
        svc_set_num_threads(nlmsvc_serv, NULL, 0);
-       svc_put(nlmsvc_serv);
        timer_delete_sync(&nlmsvc_retry);
-       nlmsvc_serv = NULL;
+       svc_destroy(&nlmsvc_serv);
        dprintk("lockd_down: service destroyed\n");
 }
 

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 4ffa1f469e90a954c28b9e562722bd4df78fd69a..760d27dd7225e94c2690d8404e678fcd58a45686 100644 (file)
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -187,7 +187,7 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
         * Check whether we're already up and running.
         */
        if (cb_info->serv)
-               return svc_get(cb_info->serv);
+               return cb_info->serv;
 
        /*
         * Sanity check: if there's no task,
@@ -245,9 +245,10 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 
        cb_info->users++;
 err_net:
-       if (!cb_info->users)
-               cb_info->serv = NULL;
-       svc_put(serv);
+       if (!cb_info->users) {
+               svc_set_num_threads(cb_info->serv, NULL, 0);
+               svc_destroy(&cb_info->serv);
+       }
 err_create:
        mutex_unlock(&nfs_callback_mutex);
        return ret;
@@ -271,11 +272,9 @@ void nfs_callback_down(int minorversion, struct net *net)
        nfs_callback_down_net(minorversion, serv, net);
        cb_info->users--;
        if (cb_info->users == 0) {
-               svc_get(serv);
                svc_set_num_threads(serv, NULL, 0);
-               svc_put(serv);
                dprintk("nfs_callback_down: service destroyed\n");
-               cb_info->serv = NULL;
+               svc_destroy(&cb_info->serv);
        }
        mutex_unlock(&nfs_callback_mutex);
 }

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 16dbef245dbbb3578f40471616bcd98a4e4c5df1..74b4360779a1121b011c131167dea866ea8d0562 100644 (file)
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -126,13 +126,6 @@ struct nfsd_net {
        struct svc_info nfsd_info;
 #define nfsd_serv nfsd_info.serv
 
-       /* When a listening socket is added to nfsd, keep_active is set
-        * and this justifies a reference on nfsd_serv.  This stops
-        * nfsd_serv from being freed.  When the number of threads is
-        * set, keep_active is cleared and the reference is dropped.  So
-        * when the last thread exits, the service will be destroyed.
-        */
-       int keep_active;
 
        /*
         * clientid and stateid data for construction of net unique COPY

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 46a001e81b55ebb87ebad5f4ed817dd9b96c5864..5c2fae98d98a1fed1ead93f3d44321b3be423782 100644 (file)
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -711,12 +711,8 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
        serv = nn->nfsd_serv;
        err = svc_addsock(serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
 
-       if (err < 0 && !serv->sv_nrthreads && !nn->keep_active)
+       if (!serv->sv_nrthreads && list_empty(&nn->nfsd_serv->sv_permsocks))
                nfsd_last_thread(net);
-       else if (err >= 0 && !serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
-               svc_get(serv);
-
-       svc_put(serv);
        return err;
 }
 
@@ -754,10 +750,6 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
        if (err < 0 && err != -EAFNOSUPPORT)
                goto out_close;
 
-       if (!serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
-               svc_get(serv);
-
-       svc_put(serv);
        return 0;
 out_close:
        xprt = svc_find_xprt(serv, transport, net, PF_INET, port);
@@ -766,10 +758,9 @@ out_close:
                svc_xprt_put(xprt);
        }
 out_err:
-       if (!serv->sv_nrthreads && !nn->keep_active)
+       if (!serv->sv_nrthreads && list_empty(&nn->nfsd_serv->sv_permsocks))
                nfsd_last_thread(net);
 
-       svc_put(serv);
        return err;
 }
 

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 365968737923cc08caecca1dd9120265356fc0aa..1a295c40934339fda63b6d49ccee961042397c90 100644 (file)
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -59,15 +59,6 @@ static __be32                        nfsd_init_request(struct svc_rqst *,
  * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and some members
  * of the svc_serv struct such as ->sv_temp_socks and ->sv_permsocks.
  *
- * If (out side the lock) nn->nfsd_serv is non-NULL, then it must point to a
- * properly initialised 'struct svc_serv' with ->sv_nrthreads > 0 (unless
- * nn->keep_active is set).  That number of nfsd threads must
- * exist and each must be listed in ->sp_all_threads in some entry of
- * ->sv_pools[].
- *
- * Each active thread holds a counted reference on nn->nfsd_serv, as does
- * the nn->keep_active flag and various transient calls to svc_get().
- *
  * Finally, the nfsd_mutex also protects some of the global variables that are
  * accessed when nfsd starts and that are settable via the write_* routines in
  * nfsctl.c. In particular:
@@ -572,6 +563,7 @@ void nfsd_last_thread(struct net *net)
 
        nfsd_shutdown_net(net);
        nfsd_export_flush(net);
+       svc_destroy(&serv);
 }
 
 void nfsd_reset_versions(struct nfsd_net *nn)
@@ -646,11 +638,9 @@ void nfsd_shutdown_threads(struct net *net)
                return;
        }
 
-       svc_get(serv);
        /* Kill outstanding nfsd threads */
        svc_set_num_threads(serv, NULL, 0);
        nfsd_last_thread(net);
-       svc_put(serv);
        mutex_unlock(&nfsd_mutex);
 }
 
@@ -666,10 +656,9 @@ int nfsd_create_serv(struct net *net)
        struct svc_serv *serv;
 
        WARN_ON(!mutex_is_locked(&nfsd_mutex));
-       if (nn->nfsd_serv) {
-               svc_get(nn->nfsd_serv);
+       if (nn->nfsd_serv)
                return 0;
-       }
+
        if (nfsd_max_blksize == 0)
                nfsd_max_blksize = nfsd_get_default_max_blksize();
        nfsd_reset_versions(nn);
@@ -680,7 +669,7 @@ int nfsd_create_serv(struct net *net)
        serv->sv_maxconn = nn->max_connections;
        error = svc_bind(serv, net);
        if (error < 0) {
-               svc_put(serv);
+               svc_destroy(&serv);
                return error;
        }
        spin_lock(&nfsd_notifier_lock);
@@ -764,7 +753,6 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
                nthreads[0] = 1;
 
        /* apply the new numbers */
-       svc_get(nn->nfsd_serv);
        for (i = 0; i < n; i++) {
                err = svc_set_num_threads(nn->nfsd_serv,
                                          &nn->nfsd_serv->sv_pools[i],
@@ -772,7 +760,6 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
                if (err)
                        break;
        }
-       svc_put(nn->nfsd_serv);
        return err;
 }
 
@@ -814,13 +801,8 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
                goto out_put;
        error = serv->sv_nrthreads;
 out_put:
-       /* Threads now hold service active */
-       if (xchg(&nn->keep_active, 0))
-               svc_put(serv);
-
        if (serv->sv_nrthreads == 0)
                nfsd_last_thread(net);
-       svc_put(serv);
 out:
        mutex_unlock(&nfsd_mutex);
        return error;

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 3bea2840272d3c378517dfb5116a5bea9d243ecc..8d7888234e9e47216cc70e68b013187f3ba6de59 100644 (file)
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -69,7 +69,6 @@ struct svc_serv {
        struct svc_program *    sv_program;     /* RPC program */
        struct svc_stat *       sv_stats;       /* RPC statistics */
        spinlock_t              sv_lock;
-       struct kref             sv_refcnt;
        unsigned int            sv_nrthreads;   /* # of server threads */
        unsigned int            sv_maxconn;     /* max connections allowed or
                                                 * '0' causing max to be based
@@ -103,31 +102,7 @@ struct svc_info {
        struct mutex            *mutex;
 };
 
-/**
- * svc_get() - increment reference count on a SUNRPC serv
- * @serv:  the svc_serv to have count incremented
- *
- * Returns: the svc_serv that was passed in.
- */
-static inline struct svc_serv *svc_get(struct svc_serv *serv)
-{
-       kref_get(&serv->sv_refcnt);
-       return serv;
-}
-
-void svc_destroy(struct kref *);
-
-/**
- * svc_put - decrement reference count on a SUNRPC serv
- * @serv:  the svc_serv to have count decremented
- *
- * When the reference count reaches zero, svc_destroy()
- * is called to clean up and free the serv.
- */
-static inline void svc_put(struct svc_serv *serv)
-{
-       kref_put(&serv->sv_refcnt, svc_destroy);
-}
+void svc_destroy(struct svc_serv **svcp);
 
 /*
  * Maximum payload size supported by a kernel RPC server.

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index fa4e23fa0e0911b21286741fad297901c9b71062..eb5856e1351d14833519eb681dc60e4f57d171cf 100644 (file)
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -463,7 +463,6 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
                return NULL;
        serv->sv_name      = prog->pg_name;
        serv->sv_program   = prog;
-       kref_init(&serv->sv_refcnt);
        serv->sv_stats     = prog->pg_stats;
        if (bufsize > RPCSVC_MAXPAYLOAD)
                bufsize = RPCSVC_MAXPAYLOAD;
@@ -564,11 +563,13 @@ EXPORT_SYMBOL_GPL(svc_create_pooled);
  * protect sv_permsocks and sv_tempsocks.
  */
 void
-svc_destroy(struct kref *ref)
+svc_destroy(struct svc_serv **servp)
 {
-       struct svc_serv *serv = container_of(ref, struct svc_serv, sv_refcnt);
+       struct svc_serv *serv = *servp;
        unsigned int i;
 
+       *servp = NULL;
+
        dprintk("svc: svc_destroy(%s)\n", serv->sv_program->pg_name);
        timer_shutdown_sync(&serv->sv_temptimer);
 
@@ -675,7 +676,6 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node)
        if (!rqstp)
                return ERR_PTR(-ENOMEM);
 
-       svc_get(serv);
        spin_lock_bh(&serv->sv_lock);
        serv->sv_nrthreads += 1;
        spin_unlock_bh(&serv->sv_lock);
@@ -935,11 +935,6 @@ svc_exit_thread(struct svc_rqst *rqstp)
 
        svc_rqst_free(rqstp);
 
-       svc_put(serv);
-       /* That svc_put() cannot be the last, because the thread
-        * waiting for SP_VICTIM_REMAINS to clear must hold
-        * a reference. So it is still safe to access pool.
-        */
        clear_and_wake_up_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
 }
 EXPORT_SYMBOL_GPL(svc_exit_thread);