nfs_localio: change nfsd_file_put_local() to take a pointer to __rcu pointer

Instead of calling xchg() and unrcu_pointer() before
nfsd_file_put_local(), we now pass pointer to the __rcu pointer and call
xchg() and unrcu_pointer() inside that function.

Where unrcu_pointer() is currently called the internals of "struct
nfsd_file" are not known and that causes older compilers such as gcc-8
to complain.

In some cases we have a __kernel (aka normal) pointer not an __rcu
pointer so we need to cast it to __rcu first.  This is strictly a
weakening so no information is lost.  Somewhat surprisingly, this cast
is accepted by gcc-8.

This has the pleasing result that the cmpxchg() which sets ro_file and
rw_file, and also the xchg() which clears them, are both now in the nfsd
code.

Reported-by: Pali Rohár <pali@kernel.org>
Reported-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Fixes: 86e00412254a ("nfs: cache all open LOCALIO nfsd_file(s) in client")
Signed-off-by: NeilBrown <neil@brown.name>
Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index ef12dd279539a5ad1e0f3a918cd93ada7d99ff75..510d0a16cfe917da9d04cf37d571d6e150f87a10 100644 (file)
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -209,9 +209,16 @@ void nfs_local_probe_async(struct nfs_client *clp)
 }
 EXPORT_SYMBOL_GPL(nfs_local_probe_async);
 
-static inline void nfs_local_file_put(struct nfsd_file *nf)
+static inline void nfs_local_file_put(struct nfsd_file *localio)
 {
-       nfs_to_nfsd_file_put_local(nf);
+       /* nfs_to_nfsd_file_put_local() expects an __rcu pointer
+        * but we have a __kernel pointer.  It is always safe
+        * to cast a __kernel pointer to an __rcu pointer
+        * because the cast only weakens what is known about the pointer.
+        */
+       struct nfsd_file __rcu *nf = (struct nfsd_file __rcu*) localio;
+
+       nfs_to_nfsd_file_put_local(&nf);
 }
 
 /*

diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
index 1dd5a8cca064d84e5089fa1fb99ca13c51a891fd..05c7c16e37ab4c431719c30d789e5cf793442de4 100644 (file)
--- a/fs/nfs_common/nfslocalio.c
+++ b/fs/nfs_common/nfslocalio.c
@@ -170,9 +170,6 @@ static bool nfs_uuid_put(nfs_uuid_t *nfs_uuid)
        while ((nfl = list_first_entry_or_null(&nfs_uuid->files,
                                               struct nfs_file_localio,
                                               list)) != NULL) {
-               struct nfsd_file *ro_nf;
-               struct nfsd_file *rw_nf;
-
                /* If nfs_uuid is already NULL, nfs_close_local_fh is
                 * closing and we must wait, else we unlink and close.
                 */
@@ -189,17 +186,14 @@ static bool nfs_uuid_put(nfs_uuid_t *nfs_uuid)
                        continue;
                }
 
-               ro_nf = unrcu_pointer(xchg(&nfl->ro_file, NULL));
-               rw_nf = unrcu_pointer(xchg(&nfl->rw_file, NULL));
-
                /* Remove nfl from nfs_uuid->files list */
                list_del_init(&nfl->list);
                spin_unlock(&nfs_uuid->lock);
-               if (ro_nf)
-                       nfs_to_nfsd_file_put_local(ro_nf);
-               if (rw_nf)
-                       nfs_to_nfsd_file_put_local(rw_nf);
+
+               nfs_to_nfsd_file_put_local(&nfl->ro_file);
+               nfs_to_nfsd_file_put_local(&nfl->rw_file);
                cond_resched();
+
                spin_lock(&nfs_uuid->lock);
                /* Now we can allow racing nfs_close_local_fh() to
                 * skip the locking.
@@ -303,8 +297,6 @@ EXPORT_SYMBOL_GPL(nfs_open_local_fh);
 
 void nfs_close_local_fh(struct nfs_file_localio *nfl)
 {
-       struct nfsd_file *ro_nf;
-       struct nfsd_file *rw_nf;
        nfs_uuid_t *nfs_uuid;
 
        rcu_read_lock();
@@ -337,12 +329,8 @@ void nfs_close_local_fh(struct nfs_file_localio *nfl)
        spin_unlock(&nfs_uuid->lock);
        rcu_read_unlock();
 
-       ro_nf = unrcu_pointer(xchg(&nfl->ro_file, NULL));
-       rw_nf = unrcu_pointer(xchg(&nfl->rw_file, NULL));
-       if (ro_nf)
-               nfs_to_nfsd_file_put_local(ro_nf);
-       if (rw_nf)
-               nfs_to_nfsd_file_put_local(rw_nf);
+       nfs_to_nfsd_file_put_local(&nfl->ro_file);
+       nfs_to_nfsd_file_put_local(&nfl->rw_file);
 
        /* Remove nfl from nfs_uuid->files list and signal nfs_uuid_put()
         * that we are done.  The moment we drop the spinlock the

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index eedf2af8ee6ec1b8e82600d8f2b7b8563d88166d..e108b6c705b4590a6d1f3d8cf75c02c4e193f0e3 100644 (file)
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -378,11 +378,16 @@ nfsd_file_put(struct nfsd_file *nf)
  * the reference of the nfsd_file.
  */
 struct net *
-nfsd_file_put_local(struct nfsd_file *nf)
+nfsd_file_put_local(struct nfsd_file __rcu **pnf)
 {
-       struct net *net = nf->nf_net;
+       struct nfsd_file *nf;
+       struct net *net = NULL;
 
-       nfsd_file_put(nf);
+       nf = unrcu_pointer(xchg(pnf, NULL));
+       if (nf) {
+               net = nf->nf_net;
+               nfsd_file_put(nf);
+       }
        return net;
 }
 

diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
index cd02f91aaef13dfd83bf13ec21b6170b45dd7650..722b26c71e454a41ddf32baef6c5747fca0ec5a9 100644 (file)
--- a/fs/nfsd/filecache.h
+++ b/fs/nfsd/filecache.h
@@ -62,7 +62,7 @@ void nfsd_file_cache_shutdown(void);
 int nfsd_file_cache_start_net(struct net *net);
 void nfsd_file_cache_shutdown_net(struct net *net);
 void nfsd_file_put(struct nfsd_file *nf);
-struct net *nfsd_file_put_local(struct nfsd_file *nf);
+struct net *nfsd_file_put_local(struct nfsd_file __rcu **nf);
 struct nfsd_file *nfsd_file_get_local(struct nfsd_file *nf);
 struct nfsd_file *nfsd_file_get(struct nfsd_file *nf);
 struct file *nfsd_file_file(struct nfsd_file *nf);

diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
index c3f34bae60e13be3f8839f4f46ac1e2ee1e14e6d..5c7c92659e736fa916ed227f99d5ec35315fce83 100644 (file)
--- a/include/linux/nfslocalio.h
+++ b/include/linux/nfslocalio.h
@@ -50,10 +50,6 @@ void nfs_localio_invalidate_clients(struct list_head *nn_local_clients,
                                    spinlock_t *nn_local_clients_lock);
 
 /* localio needs to map filehandle -> struct nfsd_file */
-extern struct nfsd_file *
-nfsd_open_local_fh(struct net *, struct auth_domain *, struct rpc_clnt *,
-                  const struct cred *, const struct nfs_fh *,
-                  const fmode_t) __must_hold(rcu);
 void nfs_close_local_fh(struct nfs_file_localio *);
 
 struct nfsd_localio_operations {
@@ -64,8 +60,9 @@ struct nfsd_localio_operations {
                                                struct rpc_clnt *,
                                                const struct cred *,
                                                const struct nfs_fh *,
+                                               struct nfsd_file __rcu **pnf,
                                                const fmode_t);
-       struct net *(*nfsd_file_put_local)(struct nfsd_file *);
+       struct net *(*nfsd_file_put_local)(struct nfsd_file __rcu **);
        struct nfsd_file *(*nfsd_file_get_local)(struct nfsd_file *);
        struct file *(*nfsd_file_file)(struct nfsd_file *);
 } ____cacheline_aligned;
@@ -76,6 +73,7 @@ extern const struct nfsd_localio_operations *nfs_to;
 struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *,
                   struct rpc_clnt *, const struct cred *,
                   const struct nfs_fh *, struct nfs_file_localio *,
+                  struct nfsd_file __rcu **pnf,
                   const fmode_t);
 
 static inline void nfs_to_nfsd_net_put(struct net *net)
@@ -90,16 +88,19 @@ static inline void nfs_to_nfsd_net_put(struct net *net)
        rcu_read_unlock();
 }
 
-static inline void nfs_to_nfsd_file_put_local(struct nfsd_file *localio)
+static inline void nfs_to_nfsd_file_put_local(struct nfsd_file __rcu **localio)
 {
        /*
-        * Must not hold RCU otherwise nfsd_file_put() can easily trigger:
-        * "Voluntary context switch within RCU read-side critical section!"
-        * by scheduling deep in underlying filesystem (e.g. XFS).
+        * Either *localio must be guaranteed to be non-NULL, or caller
+        * must prevent nfsd shutdown from completing as nfs_close_local_fh()
+        * does by blocking the nfs_uuid from being finally put.
         */
-       struct net *net = nfs_to->nfsd_file_put_local(localio);
+       struct net *net;
 
-       nfs_to_nfsd_net_put(net);
+       net = nfs_to->nfsd_file_put_local(localio);
+
+       if (net)
+               nfs_to_nfsd_net_put(net);
 }
 
 #else   /* CONFIG_NFS_LOCALIO */