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@xxxxxxxxxx>
Reported-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
Fixes: 86e00412254a ("nfs: cache all open LOCALIO nfsd_file(s) in client")
Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
---
fs/nfs/localio.c | 11 +++++++++--
fs/nfs_common/nfslocalio.c | 24 ++++++------------------
fs/nfsd/filecache.c | 11 ++++++++---
fs/nfsd/filecache.h | 2 +-
include/linux/nfslocalio.h | 17 ++++++++++-------
5 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index 030a54c8c9d8..e6d36b3d3fc0 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -207,9 +207,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 1dd5a8cca064..05c7c16e37ab 100644
--- 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 eedf2af8ee6e..e108b6c705b4 100644
--- 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 cd02f91aaef1..722b26c71e45 100644
--- 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 e6cd6ec447f5..5c7c92659e73 100644
--- a/include/linux/nfslocalio.h
+++ b/include/linux/nfslocalio.h
@@ -62,7 +62,7 @@ struct nfsd_localio_operations {
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;
@@ -88,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 */
--
2.49.0
