LinuxLists.cc - [PATCH 0/6] support admin-revocation of v4 state

2023-10-27 01:56:34

Subject: [PATCH 0/6] support admin-revocation of v4 state

This is a revised version of a patch set I sent over a year ago.
It now supports v4.0 and has had more testing.

There are cirsumstances where an admin might need to unmount a
filesystem that is NFS-exported and in active use, but does not want to
stop the NFS server completely. These are certainly unusual
circumstance and doing this might negatively impact any clients acting
on the filesystem, but the admin should be able to do this.

Currently this is quite possible for for NFSv3. Unexporting the
filesystem will ensure no new opens happen, and writing the path name to
/proc/fs/nfsd/unlock_filesystem will ensure anly NLM locks held in the
filesystem are released so that NFSD no longer prevents the filesystem
from being unlocked.

It is not currently possible for NFSv4. Writing to unlock_filesystem
does not affect NFSv4, which is arguably a bug. This series fixes the bug.

For NFSv4.1 and later code is straight forward. We add new state types
for admin-revoked state (open, lock, deleg) and change the type of any
state on a filesystem - inavlidating any access and closing files as we
go. While there are any revoked states we report this to the client in
the response to SEQUENCE requests, and it will check and free any states
that need to be freed.

For NFSv4.0 it isn't quite so easy as there is no mechanism for the
client to explicitly acknowledged admin-revoked states. The approach
this patchset takes is to discard NFSv4.0 admin-revoked states one
lease-time after they were revoked, or immediately for a state that the
client tryies to use and gets an "ADMIN_REVOKED" error for. If the
filestystem has been unmounted (as expected), the client will see STATE
errors before it has a chance to see ADMIN_REVOKED errors, so most often
the timeout will be how states are discarded.

NeilBrown

[PATCH 1/6] nfsd: prepare for supporting admin-revocation of state
[PATCH 2/6] nfsd: allow admin-revoked state to appear in
[PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.
[PATCH 4/6] nfsd: allow lock state ids to be revoked and then freed
[PATCH 5/6] nfsd: allow open state ids to be revoked and then freed
[PATCH 6/6] nfsd: allow delegation state ids to be revoked and then

2023-10-27 01:56:43

by NeilBrown

[permalink] [raw]

Subject: [PATCH 1/6] nfsd: prepare for supporting admin-revocation of state

The NFSv4 protocol allows state to be revoked by the admin and has error
codes which allow this to be communicated to the client.

This patch
- introduces 3 new state-id types for revoked open, lock, and
delegation state. This requires the bitmask to be 'short',
not 'char'
- reports NFS4ERR_ADMIN_REVOKED when these are accessed
- introduces a per-client counter of these states and returns
SEQ4_STATUS_ADMIN_STATE_REVOKED when the counter is not zero.
Decrement this when freeing any admin-revoked state.
- introduces stub code to find all interesting states for a given
superblock so they can be revoked via the 'unlock_filesystem'
file in /proc/fs/nfsd/
No actual states are handled yet.

Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfs4layouts.c | 2 +-
fs/nfsd/nfs4state.c | 93 +++++++++++++++++++++++++++++++++++++++----
fs/nfsd/nfsctl.c | 1 +
fs/nfsd/nfsd.h | 1 +
fs/nfsd/state.h | 29 +++++++++-----
fs/nfsd/trace.h | 8 +++-
6 files changed, 114 insertions(+), 20 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 5e8096bc5eaa..09d0363bfbc4 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -269,7 +269,7 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
{
struct nfs4_layout_stateid *ls;
struct nfs4_stid *stid;
- unsigned char typemask = NFS4_LAYOUT_STID;
+ unsigned short typemask = NFS4_LAYOUT_STID;
__be32 status;

if (create)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 65fd5510323a..f3ba53a16105 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1202,6 +1202,13 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
return NULL;
}

+void nfs4_unhash_stid(struct nfs4_stid *s)
+{
+ if (s->sc_type & NFS4_ALL_ADMIN_REVOKED_STIDS)
+ atomic_dec(&s->sc_client->cl_admin_revoked);
+ s->sc_type = 0;
+}
+
void
nfs4_put_stid(struct nfs4_stid *s)
{
@@ -1215,6 +1222,7 @@ nfs4_put_stid(struct nfs4_stid *s)
return;
}
idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
+ nfs4_unhash_stid(s);
nfs4_free_cpntf_statelist(clp->net, s);
spin_unlock(&clp->cl_lock);
s->sc_free(s);
@@ -1265,11 +1273,6 @@ static void destroy_unhashed_deleg(struct nfs4_delegation *dp)
nfs4_put_stid(&dp->dl_stid);
}

-void nfs4_unhash_stid(struct nfs4_stid *s)
-{
- s->sc_type = 0;
-}
-
/**
* nfs4_delegation_exists - Discover if this delegation already exists
* @clp: a pointer to the nfs4_client we're granting a delegation to
@@ -1536,6 +1539,7 @@ static void put_ol_stateid_locked(struct nfs4_ol_stateid *stp,
}

idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
+ nfs4_unhash_stid(s);
list_add(&stp->st_locks, reaplist);
}

@@ -1680,6 +1684,53 @@ static void release_openowner(struct nfs4_openowner *oo)
nfs4_put_stateowner(&oo->oo_owner);
}

+static struct nfs4_stid *find_one_sb_stid(struct nfs4_client *clp,
+ struct super_block *sb,
+ unsigned short sc_types)
+{
+ unsigned long id, tmp;
+ struct nfs4_stid *stid;
+
+ spin_lock(&clp->cl_lock);
+ idr_for_each_entry_ul(&clp->cl_stateids, stid, tmp, id)
+ if ((stid->sc_type & sc_types) &&
+ stid->sc_file->fi_inode->i_sb == sb) {
+ refcount_inc(&stid->sc_count);
+ break;
+ }
+ spin_unlock(&clp->cl_lock);
+ return stid;
+}
+
+void nfsd4_revoke_states(struct net *net, struct super_block *sb)
+{
+ struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+ unsigned int idhashval;
+ unsigned short sc_types;
+
+ sc_types = 0;
+
+ spin_lock(&nn->client_lock);
+ for (idhashval = 0; idhashval < CLIENT_HASH_MASK; idhashval++) {
+ struct list_head *head = &nn->conf_id_hashtbl[idhashval];
+ struct nfs4_client *clp;
+ retry:
+ list_for_each_entry(clp, head, cl_idhash) {
+ struct nfs4_stid *stid = find_one_sb_stid(clp, sb,
+ sc_types);
+ if (stid) {
+ spin_unlock(&nn->client_lock);
+ switch (stid->sc_type) {
+ }
+ nfs4_put_stid(stid);
+ spin_lock(&nn->client_lock);
+ goto retry;
+ }
+ }
+ }
+ spin_unlock(&nn->client_lock);
+}
+
static inline int
hash_sessionid(struct nfs4_sessionid *sessionid)
{
@@ -2465,7 +2516,8 @@ find_stateid_locked(struct nfs4_client *cl, stateid_t *t)
}

static struct nfs4_stid *
-find_stateid_by_type(struct nfs4_client *cl, stateid_t *t, char typemask)
+find_stateid_by_type(struct nfs4_client *cl, stateid_t *t,
+ unsigned short typemask)
{
struct nfs4_stid *s;

@@ -2549,6 +2601,8 @@ static int client_info_show(struct seq_file *m, void *v)
}
seq_printf(m, "callback state: %s\n", cb_state2str(clp->cl_cb_state));
seq_printf(m, "callback address: %pISpc\n", &clp->cl_cb_conn.cb_addr);
+ seq_printf(m, "admin-revoked states: %d\n",
+ atomic_read(&clp->cl_admin_revoked));
drop_client(clp);

return 0;
@@ -4108,6 +4162,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
if (!list_empty(&clp->cl_revoked))
seq->status_flags |= SEQ4_STATUS_RECALLABLE_STATE_REVOKED;
+ if (atomic_read(&clp->cl_admin_revoked))
+ seq->status_flags |= SEQ4_STATUS_ADMIN_STATE_REVOKED;
out_no_session:
if (conn)
free_conn(conn);
@@ -5200,6 +5256,11 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
status = nfserr_deleg_revoked;
goto out;
}
+ if (deleg->dl_stid.sc_type == NFS4_ADMIN_REVOKED_DELEG_STID) {
+ nfs4_put_stid(&deleg->dl_stid);
+ status = nfserr_admin_revoked;
+ goto out;
+ }
flags = share_access_to_flags(open->op_share_access);
status = nfs4_check_delegmode(deleg, flags);
if (status) {
@@ -6478,6 +6539,11 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
case NFS4_REVOKED_DELEG_STID:
status = nfserr_deleg_revoked;
break;
+ case NFS4_ADMIN_REVOKED_STID:
+ case NFS4_ADMIN_REVOKED_LOCK_STID:
+ case NFS4_ADMIN_REVOKED_DELEG_STID:
+ status = nfserr_admin_revoked;
+ break;
case NFS4_OPEN_STID:
case NFS4_LOCK_STID:
status = nfsd4_check_openowner_confirmed(openlockstateid(s));
@@ -6496,7 +6562,7 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)

__be32
nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
- stateid_t *stateid, unsigned char typemask,
+ stateid_t *stateid, unsigned short typemask,
struct nfs4_stid **s, struct nfsd_net *nn)
{
__be32 status;
@@ -6512,6 +6578,13 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
else if (typemask & NFS4_DELEG_STID)
typemask |= NFS4_REVOKED_DELEG_STID;

+ if (typemask & NFS4_OPEN_STID)
+ typemask |= NFS4_ADMIN_REVOKED_STID;
+ if (typemask & NFS4_LOCK_STID)
+ typemask |= NFS4_ADMIN_REVOKED_LOCK_STID;
+ if (typemask & NFS4_DELEG_STID)
+ typemask |= NFS4_ADMIN_REVOKED_DELEG_STID;
+
if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
CLOSE_STATEID(stateid))
return nfserr_bad_stateid;
@@ -6532,6 +6605,10 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
return nfserr_deleg_revoked;
return nfserr_bad_stateid;
}
+ if (stid->sc_type & NFS4_ALL_ADMIN_REVOKED_STIDS) {
+ nfs4_put_stid(stid);
+ return nfserr_admin_revoked;
+ }
*s = stid;
return nfs_ok;
}
@@ -6899,7 +6976,7 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
*/
static __be32
nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
- stateid_t *stateid, char typemask,
+ stateid_t *stateid, unsigned short typemask,
struct nfs4_ol_stateid **stpp,
struct nfsd_net *nn)
{
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 739ed5bf71cd..50368eec86b0 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -281,6 +281,7 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
* 3. Is that directory the root of an exported file system?
*/
error = nlmsvc_unlock_all_by_sb(path.dentry->d_sb);
+ nfsd4_revoke_states(netns(file), path.dentry->d_sb);

path_put(&path);
return error;
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index f5ff42f41ee7..d46203eac3c8 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -280,6 +280,7 @@ void nfsd_lockd_shutdown(void);
#define nfserr_no_grace cpu_to_be32(NFSERR_NO_GRACE)
#define nfserr_reclaim_bad cpu_to_be32(NFSERR_RECLAIM_BAD)
#define nfserr_badname cpu_to_be32(NFSERR_BADNAME)
+#define nfserr_admin_revoked cpu_to_be32(NFS4ERR_ADMIN_REVOKED)
#define nfserr_cb_path_down cpu_to_be32(NFSERR_CB_PATH_DOWN)
#define nfserr_locked cpu_to_be32(NFSERR_LOCKED)
#define nfserr_wrongsec cpu_to_be32(NFSERR_WRONGSEC)
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index f96eaa8e9413..6150b84827d6 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -88,17 +88,23 @@ struct nfsd4_callback_ops {
*/
struct nfs4_stid {
refcount_t sc_count;
-#define NFS4_OPEN_STID 1
-#define NFS4_LOCK_STID 2
-#define NFS4_DELEG_STID 4
+ struct list_head sc_cp_list;
+ unsigned short sc_type;
+#define NFS4_OPEN_STID BIT(0)
+#define NFS4_LOCK_STID BIT(1)
+#define NFS4_DELEG_STID BIT(2)
/* For an open stateid kept around *only* to process close replays: */
-#define NFS4_CLOSED_STID 8
+#define NFS4_CLOSED_STID BIT(3)
/* For a deleg stateid kept around only to process free_stateid's: */
-#define NFS4_REVOKED_DELEG_STID 16
-#define NFS4_CLOSED_DELEG_STID 32
-#define NFS4_LAYOUT_STID 64
- struct list_head sc_cp_list;
- unsigned char sc_type;
+#define NFS4_REVOKED_DELEG_STID BIT(4)
+#define NFS4_CLOSED_DELEG_STID BIT(5)
+#define NFS4_LAYOUT_STID BIT(6)
+#define NFS4_ADMIN_REVOKED_STID BIT(7)
+#define NFS4_ADMIN_REVOKED_LOCK_STID BIT(8)
+#define NFS4_ADMIN_REVOKED_DELEG_STID BIT(9)
+#define NFS4_ALL_ADMIN_REVOKED_STIDS (NFS4_ADMIN_REVOKED_STID | \
+ NFS4_ADMIN_REVOKED_LOCK_STID | \
+ NFS4_ADMIN_REVOKED_DELEG_STID)
stateid_t sc_stateid;
spinlock_t sc_lock;
struct nfs4_client *sc_client;
@@ -372,6 +378,7 @@ struct nfs4_client {
clientid_t cl_clientid; /* generated by server */
nfs4_verifier cl_confirm; /* generated by server */
u32 cl_minorversion;
+ atomic_t cl_admin_revoked; /* count of admin-revoked states */
/* NFSv4.1 client implementation id: */
struct xdr_netobj cl_nii_domain;
struct xdr_netobj cl_nii_name;
@@ -694,7 +701,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
stateid_t *stateid, int flags, struct nfsd_file **filp,
struct nfs4_stid **cstid);
__be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
- stateid_t *stateid, unsigned char typemask,
+ stateid_t *stateid, unsigned short typemask,
struct nfs4_stid **s, struct nfsd_net *nn);
struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *slab,
void (*sc_free)(struct nfs4_stid *));
@@ -736,6 +743,8 @@ static inline void get_nfs4_file(struct nfs4_file *fi)
}
struct nfsd_file *find_any_file(struct nfs4_file *f);

+void nfsd4_revoke_states(struct net *net, struct super_block *sb);
+
/* grace period management */
void nfsd4_end_grace(struct nfsd_net *nn);

diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index fbc0ccb40424..e359d531402c 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -648,6 +648,9 @@ TRACE_DEFINE_ENUM(NFS4_CLOSED_STID);
TRACE_DEFINE_ENUM(NFS4_REVOKED_DELEG_STID);
TRACE_DEFINE_ENUM(NFS4_CLOSED_DELEG_STID);
TRACE_DEFINE_ENUM(NFS4_LAYOUT_STID);
+TRACE_DEFINE_ENUM(NFS4_ADMIN_REVOKED_STID);
+TRACE_DEFINE_ENUM(NFS4_ADMIN_REVOKED_LOCK_STID);
+TRACE_DEFINE_ENUM(NFS4_ADMIN_REVOKED_DELEG_STID);

#define show_stid_type(x) \
__print_flags(x, "|", \
@@ -657,7 +660,10 @@ TRACE_DEFINE_ENUM(NFS4_LAYOUT_STID);
{ NFS4_CLOSED_STID, "CLOSED" }, \
{ NFS4_REVOKED_DELEG_STID, "REVOKED" }, \
{ NFS4_CLOSED_DELEG_STID, "CLOSED_DELEG" }, \
- { NFS4_LAYOUT_STID, "LAYOUT" })
+ { NFS4_LAYOUT_STID, "LAYOUT" }, \
+ { NFS4_ADMIN_REVOKED_STID, "ADMIN_REVOKED" }, \
+ { NFS4_ADMIN_REVOKED_LOCK_STID, "ADMIN_REVOKED_LOCK" }, \
+ { NFS4_ADMIN_REVOKED_DELEG_STID,"ADMIN_REVOKED_DELEG" })

DECLARE_EVENT_CLASS(nfsd_stid_class,
TP_PROTO(
--
2.42.0

2023-10-27 01:56:49

by NeilBrown

[permalink] [raw]

Subject: [PATCH 2/6] nfsd: allow admin-revoked state to appear in /proc/fs/nfsd/clients/*/states

Change the "show" functions to show some content even if a file cannot
be found, and call them for the ADMIN_REVOKED versions of state id.
This use primarily useful for debugging - to ensure states are being
removed eventually.

Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfs4state.c | 78 ++++++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f3ba53a16105..e15d35c57991 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2679,17 +2679,10 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
struct nfs4_stateowner *oo;
unsigned int access, deny;

- if (st->sc_type != NFS4_OPEN_STID && st->sc_type != NFS4_LOCK_STID)
- return 0; /* XXX: or SEQ_SKIP? */
ols = openlockstateid(st);
oo = ols->st_stateowner;
nf = st->sc_file;

- spin_lock(&nf->fi_lock);
- file = find_any_file_locked(nf);
- if (!file)
- goto out;
-
seq_printf(s, "- ");
nfs4_show_stateid(s, &st->sc_stateid);
seq_printf(s, ": { type: open, ");
@@ -2704,14 +2697,17 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
deny & NFS4_SHARE_ACCESS_READ ? "r" : "-",
deny & NFS4_SHARE_ACCESS_WRITE ? "w" : "-");

- nfs4_show_superblock(s, file);
- seq_printf(s, ", ");
- nfs4_show_fname(s, file);
- seq_printf(s, ", ");
+ spin_lock(&nf->fi_lock);
+ file = find_any_file_locked(nf);
+ if (file) {
+ nfs4_show_superblock(s, file);
+ seq_puts(s, ", ");
+ nfs4_show_fname(s, file);
+ seq_puts(s, ", ");
+ }
+ spin_unlock(&nf->fi_lock);
nfs4_show_owner(s, oo);
seq_printf(s, " }\n");
-out:
- spin_unlock(&nf->fi_lock);
return 0;
}

@@ -2725,30 +2721,29 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
ols = openlockstateid(st);
oo = ols->st_stateowner;
nf = st->sc_file;
- spin_lock(&nf->fi_lock);
- file = find_any_file_locked(nf);
- if (!file)
- goto out;

seq_printf(s, "- ");
nfs4_show_stateid(s, &st->sc_stateid);
seq_printf(s, ": { type: lock, ");

- /*
- * Note: a lock stateid isn't really the same thing as a lock,
- * it's the locking state held by one owner on a file, and there
- * may be multiple (or no) lock ranges associated with it.
- * (Same for the matter is true of open stateids.)
- */
+ spin_lock(&nf->fi_lock);
+ file = find_any_file_locked(nf);
+ if (file) {
+ /*
+ * Note: a lock stateid isn't really the same thing as a lock,
+ * it's the locking state held by one owner on a file, and there
+ * may be multiple (or no) lock ranges associated with it.
+ * (Same for the matter is true of open stateids.)
+ */

- nfs4_show_superblock(s, file);
- /* XXX: open stateid? */
- seq_printf(s, ", ");
- nfs4_show_fname(s, file);
- seq_printf(s, ", ");
+ nfs4_show_superblock(s, file);
+ /* XXX: open stateid? */
+ seq_puts(s, ", ");
+ nfs4_show_fname(s, file);
+ seq_puts(s, ", ");
+ }
nfs4_show_owner(s, oo);
seq_printf(s, " }\n");
-out:
spin_unlock(&nf->fi_lock);
return 0;
}
@@ -2761,27 +2756,27 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)

ds = delegstateid(st);
nf = st->sc_file;
- spin_lock(&nf->fi_lock);
- file = nf->fi_deleg_file;
- if (!file)
- goto out;

seq_printf(s, "- ");
nfs4_show_stateid(s, &st->sc_stateid);
seq_printf(s, ": { type: deleg, ");

/* Kinda dead code as long as we only support read delegs: */
- seq_printf(s, "access: %s, ",
- ds->dl_type == NFS4_OPEN_DELEGATE_READ ? "r" : "w");
+ seq_printf(s, "access: %s",
+ ds->dl_type == NFS4_OPEN_DELEGATE_READ ? "r" : "w");

/* XXX: lease time, whether it's being recalled. */

- nfs4_show_superblock(s, file);
- seq_printf(s, ", ");
- nfs4_show_fname(s, file);
- seq_printf(s, " }\n");
-out:
+ spin_lock(&nf->fi_lock);
+ file = nf->fi_deleg_file;
+ if (file) {
+ seq_puts(s, ", ");
+ nfs4_show_superblock(s, file);
+ seq_puts(s, ", ");
+ nfs4_show_fname(s, file);
+ }
spin_unlock(&nf->fi_lock);
+ seq_puts(s, " }\n");
return 0;
}

@@ -2813,10 +2808,13 @@ static int states_show(struct seq_file *s, void *v)

switch (st->sc_type) {
case NFS4_OPEN_STID:
+ case NFS4_ADMIN_REVOKED_STID:
return nfs4_show_open(s, st);
case NFS4_LOCK_STID:
+ case NFS4_ADMIN_REVOKED_LOCK_STID:
return nfs4_show_lock(s, st);
case NFS4_DELEG_STID:
+ case NFS4_ADMIN_REVOKED_DELEG_STID:
return nfs4_show_deleg(s, st);
case NFS4_LAYOUT_STID:
return nfs4_show_layout(s, st);
--
2.42.0

2023-10-27 01:56:56

by NeilBrown

[permalink] [raw]

Subject: [PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.

For NFSv4.1 and later the client easily discovers if there is any
admin-revoked state and will then find and explicitly free it.

For NFSv4.0 there is no such mechanism. The client can only find that
state is admin-revoked if it tries to use that state, and there is no
way for it to explicitly free the state. So the server must hold on to,
at least, the stateid for an indefinite amount of time. A
RELEASE_LOCKOWNER request might justify forgetting some of these
stateids, as would the whole clients lease lapsing, but these are not
reliable.

This patch takes two approaches.

Whenever a client uses an revoked stateid, that stateid is then
discarded and will not be recognised again. This might confuse a client
which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
all, but should mostly work. Hopefully one error will lead to other
resources being closed (e.g. process exits), which will result in more
stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.

Also, any admin-revoked stateids that have been that way for more than
one lease time are periodically revoke.

No actual freeing of state happens in this patch. That will come in
future patches which handle the different sorts of revoked state.

Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/netns.h | 4 ++
fs/nfsd/nfs4state.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 100 insertions(+)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index ec49b200b797..02f8fa095b0f 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -197,6 +197,10 @@ struct nfsd_net {
atomic_t nfsd_courtesy_clients;
struct shrinker nfsd_client_shrinker;
struct work_struct nfsd_shrinker_work;
+
+ /* last time an admin-revoke happened for NFSv4.0 */
+ time64_t nfs40_last_revoke;
+
};

/* Simple check to find out if a given net was properly initialized */
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e15d35c57991..cea36f3ff204 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1723,6 +1723,14 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
switch (stid->sc_type) {
}
nfs4_put_stid(stid);
+ if (clp->cl_minorversion == 0)
+ /* Allow cleanup after a lease period.
+ * store_release ensures cleanup will
+ * see any newly revoked states if it
+ * sees the time updated.
+ */
+ smp_store_release(&nn->nfs40_last_revoke,
+ ktime_get_boottime_seconds());
spin_lock(&nn->client_lock);
goto retry;
}
@@ -4645,6 +4653,39 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
return ret;
}

+static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
+{
+ struct nfs4_client *cl = s->sc_client;
+
+ switch (s->sc_type) {
+ default:
+ spin_unlock(&cl->cl_lock);
+ }
+}
+
+static void nfs40_drop_revoked_stid(struct nfs4_client *cl,
+ stateid_t *stid)
+{
+ /* NFSv4.0 has no way for the client to tell the server
+ * that it can forget an admin-revoked stateid.
+ * So we keep it around until the first time that the
+ * client uses it, and drop it the first time
+ * nfserr_admin_revoked is returned.
+ * For v4.1 and later we wait until explicitly told
+ * to free the stateid.
+ */
+ if (cl->cl_minorversion == 0) {
+ struct nfs4_stid *st;
+
+ spin_lock(&cl->cl_lock);
+ st = find_stateid_locked(cl, stid);
+ if (st)
+ nfsd_drop_revoked_stid(st);
+ else
+ spin_unlock(&cl->cl_lock);
+ }
+}
+
static __be32
nfsd4_verify_open_stid(struct nfs4_stid *s)
{
@@ -4672,6 +4713,10 @@ nfsd4_lock_ol_stateid(struct nfs4_ol_stateid *stp)

mutex_lock_nested(&stp->st_mutex, LOCK_STATEID_MUTEX);
ret = nfsd4_verify_open_stid(&stp->st_stid);
+ if (ret == nfserr_admin_revoked)
+ nfs40_drop_revoked_stid(stp->st_stid.sc_client,
+ &stp->st_stid.sc_stateid);
+
if (ret != nfs_ok)
mutex_unlock(&stp->st_mutex);
return ret;
@@ -5256,6 +5301,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
}
if (deleg->dl_stid.sc_type == NFS4_ADMIN_REVOKED_DELEG_STID) {
nfs4_put_stid(&deleg->dl_stid);
+ nfs40_drop_revoked_stid(cl, &open->op_delegate_stateid);
status = nfserr_admin_revoked;
goto out;
}
@@ -6253,6 +6299,41 @@ nfs4_process_client_reaplist(struct list_head *reaplist)
}
}

+static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
+ struct laundry_time *lt)
+{
+ struct nfs4_client *clp;
+
+ /* load_acquire avoids races with nfsd4_revoke_states() */
+ if (smp_load_acquire(&nn->nfs40_last_revoke) == 0 ||
+ nn->nfs40_last_revoke > lt->cutoff)
+ return;
+
+ nn->nfs40_last_revoke = 0;
+
+retry:
+ spin_lock(&nn->client_lock);
+ list_for_each_entry(clp, &nn->client_lru, cl_lru) {
+ unsigned long id, tmp;
+ struct nfs4_stid *stid;
+
+ if (atomic_read(&clp->cl_admin_revoked) == 0)
+ continue;
+
+ spin_lock(&clp->cl_lock);
+ idr_for_each_entry_ul(&clp->cl_stateids, stid, tmp, id)
+ if (stid->sc_type & NFS4_ALL_ADMIN_REVOKED_STIDS) {
+ refcount_inc(&stid->sc_count);
+ spin_unlock(&nn->client_lock);
+ nfsd_drop_revoked_stid(stid);
+ nfs4_put_stid(stid);
+ goto retry;
+ }
+ spin_unlock(&clp->cl_lock);
+ }
+ spin_unlock(&nn->client_lock);
+}
+
static time64_t
nfs4_laundromat(struct nfsd_net *nn)
{
@@ -6286,6 +6367,8 @@ nfs4_laundromat(struct nfsd_net *nn)
nfs4_get_client_reaplist(nn, &reaplist, &lt);
nfs4_process_client_reaplist(&reaplist);

+ nfs40_clean_admin_revoked(nn, &lt);
+
spin_lock(&state_lock);
list_for_each_safe(pos, next, &nn->del_recall_lru) {
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
@@ -6504,6 +6587,9 @@ static __be32 nfsd4_stid_check_stateid_generation(stateid_t *in, struct nfs4_sti
if (ret == nfs_ok)
ret = check_stateid_generation(in, &s->sc_stateid, has_session);
spin_unlock(&s->sc_lock);
+ if (ret == nfserr_admin_revoked)
+ nfs40_drop_revoked_stid(s->sc_client,
+ &s->sc_stateid);
return ret;
}

@@ -6555,6 +6641,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
}
out_unlock:
spin_unlock(&cl->cl_lock);
+ if (status == nfserr_admin_revoked)
+ nfs40_drop_revoked_stid(cl, stateid);
return status;
}

@@ -6604,6 +6692,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
return nfserr_bad_stateid;
}
if (stid->sc_type & NFS4_ALL_ADMIN_REVOKED_STIDS) {
+ nfs40_drop_revoked_stid(cstate->clp, stateid);
nfs4_put_stid(stid);
return nfserr_admin_revoked;
}
@@ -6923,6 +7012,13 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
nfs4_put_stid(s);
ret = nfs_ok;
goto out;
+ case NFS4_ADMIN_REVOKED_STID:
+ case NFS4_ADMIN_REVOKED_LOCK_STID:
+ case NFS4_ADMIN_REVOKED_DELEG_STID:
+ spin_unlock(&s->sc_lock);
+ nfsd_drop_revoked_stid(s);
+ ret = nfs_ok;
+ goto out;
/* Default falls through and returns nfserr_bad_stateid */
}
spin_unlock(&s->sc_lock);
--
2.42.0

2023-10-27 01:56:59

by NeilBrown

[permalink] [raw]

Subject: [PATCH 4/6] nfsd: allow lock state ids to be revoked and then freed

Revoking state through 'unlock_filesystem' now revokes any lock states
found. When the stateids are then freed by the client, the revoked
stateids will be cleaned up correctly.

Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfs4state.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cea36f3ff204..4e912c377d63 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1708,7 +1708,7 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
unsigned int idhashval;
unsigned short sc_types;

- sc_types = 0;
+ sc_types = NFS4_LOCK_STID;

spin_lock(&nn->client_lock);
for (idhashval = 0; idhashval < CLIENT_HASH_MASK; idhashval++) {
@@ -1719,8 +1719,33 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
struct nfs4_stid *stid = find_one_sb_stid(clp, sb,
sc_types);
if (stid) {
+ struct nfs4_ol_stateid *stp;
+
spin_unlock(&nn->client_lock);
switch (stid->sc_type) {
+ case NFS4_LOCK_STID:
+ stp = openlockstateid(stid);
+ mutex_lock_nested(&stp->st_mutex,
+ LOCK_STATEID_MUTEX);
+ if (stid->sc_type == NFS4_LOCK_STID) {
+ struct nfs4_lockowner *lo =
+ lockowner(stp->st_stateowner);
+ struct nfsd_file *nf;
+
+ nf = find_any_file(stp->st_stid.sc_file);
+ if (nf) {
+ get_file(nf->nf_file);
+ filp_close(nf->nf_file,
+ (fl_owner_t)lo);
+ nfsd_file_put(nf);
+ }
+ release_all_access(stp);
+ stid->sc_type =
+ NFS4_ADMIN_REVOKED_LOCK_STID;
+ atomic_inc(&clp->cl_admin_revoked);
+ }
+ mutex_unlock(&stp->st_mutex);
+ break;
}
nfs4_put_stid(stid);
if (clp->cl_minorversion == 0)
@@ -4656,8 +4681,17 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
{
struct nfs4_client *cl = s->sc_client;
+ struct nfs4_ol_stateid *stp;
+ bool unhashed;

switch (s->sc_type) {
+ case NFS4_ADMIN_REVOKED_LOCK_STID:
+ stp = openlockstateid(s);
+ unhashed = unhash_lock_stateid(stp);
+ spin_unlock(&cl->cl_lock);
+ if (unhashed)
+ nfs4_put_stid(s);
+ break;
default:
spin_unlock(&cl->cl_lock);
}
--
2.42.0

2023-10-27 01:57:06

by NeilBrown

[permalink] [raw]

Subject: [PATCH 5/6] nfsd: allow open state ids to be revoked and then freed

Revoking state through 'unlock_filesystem' now revokes any open states
found. When the stateids are then freed by the client, the revoked
stateids will be cleaned up correctly.

Possibly the related lock states should be revoked too, but a
subsequent patch will do that for all lock state on the superblock.

Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfs4state.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4e912c377d63..ee93ab5d1e0f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1708,7 +1708,7 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
unsigned int idhashval;
unsigned short sc_types;

- sc_types = NFS4_LOCK_STID;
+ sc_types = NFS4_OPEN_STID | NFS4_LOCK_STID;

spin_lock(&nn->client_lock);
for (idhashval = 0; idhashval < CLIENT_HASH_MASK; idhashval++) {
@@ -1723,6 +1723,18 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)

spin_unlock(&nn->client_lock);
switch (stid->sc_type) {
+ case NFS4_OPEN_STID:
+ stp = openlockstateid(stid);
+ mutex_lock_nested(&stp->st_mutex,
+ OPEN_STATEID_MUTEX);
+ if (stid->sc_type == NFS4_OPEN_STID) {
+ release_all_access(stp);
+ stid->sc_type =
+ NFS4_ADMIN_REVOKED_STID;
+ atomic_inc(&clp->cl_admin_revoked);
+ }
+ mutex_unlock(&stp->st_mutex);
+ break;
case NFS4_LOCK_STID:
stp = openlockstateid(stid);
mutex_lock_nested(&stp->st_mutex,
@@ -4681,10 +4693,18 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
{
struct nfs4_client *cl = s->sc_client;
+ LIST_HEAD(reaplist);
struct nfs4_ol_stateid *stp;
bool unhashed;

switch (s->sc_type) {
+ case NFS4_ADMIN_REVOKED_STID:
+ stp = openlockstateid(s);
+ if (unhash_open_stateid(stp, &reaplist))
+ put_ol_stateid_locked(stp, &reaplist);
+ spin_unlock(&cl->cl_lock);
+ free_ol_stateid_reaplist(&reaplist);
+ break;
case NFS4_ADMIN_REVOKED_LOCK_STID:
stp = openlockstateid(s);
unhashed = unhash_lock_stateid(stp);
--
2.42.0

2023-10-27 01:57:10

by NeilBrown

[permalink] [raw]

Subject: [PATCH 6/6] nfsd: allow delegation state ids to be revoked and then freed

Revoking state through 'unlock_filesystem' now revokes any delegation
states found. When the stateids are then freed by the client, the
revoked stateids will be cleaned up correctly.

As there is already support for revoking delegations, we build on that
for admin-revoking.

Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfs4state.c | 36 +++++++++++++++++++++++++++++-------
1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ee93ab5d1e0f..ccdf3beb3640 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1367,7 +1367,8 @@ static void destroy_delegation(struct nfs4_delegation *dp)
destroy_unhashed_deleg(dp);
}

-static void revoke_delegation(struct nfs4_delegation *dp)
+static void revoke_delegation(struct nfs4_delegation *dp,
+ unsigned short sc_type)
{
struct nfs4_client *clp = dp->dl_stid.sc_client;

@@ -1375,9 +1376,9 @@ static void revoke_delegation(struct nfs4_delegation *dp)

trace_nfsd_stid_revoke(&dp->dl_stid);

- if (clp->cl_minorversion) {
+ if (clp->cl_minorversion || sc_type == NFS4_ADMIN_REVOKED_DELEG_STID) {
spin_lock(&clp->cl_lock);
- dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
+ dp->dl_stid.sc_type = sc_type;
refcount_inc(&dp->dl_stid.sc_count);
list_add(&dp->dl_recall_lru, &clp->cl_revoked);
spin_unlock(&clp->cl_lock);
@@ -1708,7 +1709,7 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
unsigned int idhashval;
unsigned short sc_types;

- sc_types = NFS4_OPEN_STID | NFS4_LOCK_STID;
+ sc_types = NFS4_OPEN_STID | NFS4_LOCK_STID | NFS4_DELEG_STID;

spin_lock(&nn->client_lock);
for (idhashval = 0; idhashval < CLIENT_HASH_MASK; idhashval++) {
@@ -1720,6 +1721,7 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
sc_types);
if (stid) {
struct nfs4_ol_stateid *stp;
+ struct nfs4_delegation *dp;

spin_unlock(&nn->client_lock);
switch (stid->sc_type) {
@@ -1758,6 +1760,18 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
}
mutex_unlock(&stp->st_mutex);
break;
+ case NFS4_DELEG_STID:
+ dp = delegstateid(stid);
+ spin_lock(&state_lock);
+ if (!unhash_delegation_locked(dp))
+ dp = NULL;
+ spin_unlock(&state_lock);
+ if (dp) {
+ list_del_init(&dp->dl_recall_lru);
+ revoke_delegation(
+ dp, NFS4_ADMIN_REVOKED_DELEG_STID);
+ }
+ break;
}
nfs4_put_stid(stid);
if (clp->cl_minorversion == 0)
@@ -4695,6 +4709,7 @@ static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
struct nfs4_client *cl = s->sc_client;
LIST_HEAD(reaplist);
struct nfs4_ol_stateid *stp;
+ struct nfs4_delegation *dp;
bool unhashed;

switch (s->sc_type) {
@@ -4712,6 +4727,12 @@ static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
if (unhashed)
nfs4_put_stid(s);
break;
+ case NFS4_ADMIN_REVOKED_DELEG_STID:
+ dp = delegstateid(s);
+ list_del_init(&dp->dl_recall_lru);
+ spin_unlock(&cl->cl_lock);
+ nfs4_put_stid(s);
+ break;
default:
spin_unlock(&cl->cl_lock);
}
@@ -5073,8 +5094,9 @@ static int nfsd4_cb_recall_done(struct nfsd4_callback *cb,
trace_nfsd_cb_recall_done(&dp->dl_stid.sc_stateid, task);

if (dp->dl_stid.sc_type == NFS4_CLOSED_DELEG_STID ||
- dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID)
- return 1;
+ dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID ||
+ dp->dl_stid.sc_type == NFS4_ADMIN_REVOKED_DELEG_STID)
+ return 1;

switch (task->tk_status) {
case 0:
@@ -6436,7 +6458,7 @@ nfs4_laundromat(struct nfsd_net *nn)
dp = list_first_entry(&reaplist, struct nfs4_delegation,
dl_recall_lru);
list_del_init(&dp->dl_recall_lru);
- revoke_delegation(dp);
+ revoke_delegation(dp, NFS4_REVOKED_DELEG_STID);
}

spin_lock(&nn->client_lock);
--
2.42.0

2023-10-27 15:44:59

by Chuck Lever III

[permalink] [raw]

Subject: Re: [PATCH 0/6] support admin-revocation of v4 state

On Fri, Oct 27, 2023 at 12:45:28PM +1100, NeilBrown wrote:
> This is a revised version of a patch set I sent over a year ago.
> It now supports v4.0 and has had more testing.
>
> There are cirsumstances where an admin might need to unmount a
> filesystem that is NFS-exported and in active use, but does not want to
> stop the NFS server completely. These are certainly unusual
> circumstance and doing this might negatively impact any clients acting
> on the filesystem, but the admin should be able to do this.
>
> Currently this is quite possible for for NFSv3. Unexporting the
> filesystem will ensure no new opens happen, and writing the path name to
> /proc/fs/nfsd/unlock_filesystem will ensure anly NLM locks held in the
> filesystem are released so that NFSD no longer prevents the filesystem
> from being unlocked.
>
> It is not currently possible for NFSv4. Writing to unlock_filesystem
> does not affect NFSv4, which is arguably a bug. This series fixes the bug.

I agree that this is a good thing to do.

However, I'd like to migrate the "unlock_filesystem" functionality
to the nfsd netlink protocol first rather than adding this support
to /proc/fs/nfsd/. I don't believe that would be a difficult pre-
requisite to get through.

Does that seem sensible?

> For NFSv4.1 and later code is straight forward. We add new state types
> for admin-revoked state (open, lock, deleg) and change the type of any
> state on a filesystem - inavlidating any access and closing files as we
> go. While there are any revoked states we report this to the client in
> the response to SEQUENCE requests, and it will check and free any states
> that need to be freed.
>
> For NFSv4.0 it isn't quite so easy as there is no mechanism for the
> client to explicitly acknowledged admin-revoked states. The approach
> this patchset takes is to discard NFSv4.0 admin-revoked states one
> lease-time after they were revoked, or immediately for a state that the
> client tryies to use and gets an "ADMIN_REVOKED" error for. If the
> filestystem has been unmounted (as expected), the client will see STATE
> errors before it has a chance to see ADMIN_REVOKED errors, so most often
> the timeout will be how states are discarded.
>
> NeilBrown
>
> [PATCH 1/6] nfsd: prepare for supporting admin-revocation of state
> [PATCH 2/6] nfsd: allow admin-revoked state to appear in
> [PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.
> [PATCH 4/6] nfsd: allow lock state ids to be revoked and then freed
> [PATCH 5/6] nfsd: allow open state ids to be revoked and then freed
> [PATCH 6/6] nfsd: allow delegation state ids to be revoked and then
>

--
Chuck Lever

2023-10-27 22:11:18

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH 0/6] support admin-revocation of v4 state

On Sat, 28 Oct 2023, Chuck Lever wrote:
> On Fri, Oct 27, 2023 at 12:45:28PM +1100, NeilBrown wrote:
> > This is a revised version of a patch set I sent over a year ago.
> > It now supports v4.0 and has had more testing.
> >
> > There are cirsumstances where an admin might need to unmount a
> > filesystem that is NFS-exported and in active use, but does not want to
> > stop the NFS server completely. These are certainly unusual
> > circumstance and doing this might negatively impact any clients acting
> > on the filesystem, but the admin should be able to do this.
> >
> > Currently this is quite possible for for NFSv3. Unexporting the
> > filesystem will ensure no new opens happen, and writing the path name to
> > /proc/fs/nfsd/unlock_filesystem will ensure anly NLM locks held in the
> > filesystem are released so that NFSD no longer prevents the filesystem
> > from being unlocked.
> >
> > It is not currently possible for NFSv4. Writing to unlock_filesystem
> > does not affect NFSv4, which is arguably a bug. This series fixes the bug.
>
> I agree that this is a good thing to do.
>
> However, I'd like to migrate the "unlock_filesystem" functionality
> to the nfsd netlink protocol first rather than adding this support
> to /proc/fs/nfsd/. I don't believe that would be a difficult pre-
> requisite to get through.
>
> Does that seem sensible?

Not to me.

This is not new functionality - it is a fix for existing functionality
which incorrectly ignores NFSv4.

When you say "migrate" I hope you mean to add the "unlock_filesystem"
functionality to netlink, but not remove it from /proc/fs/nfsd for
several years at least. I certainly wouldn't want to wait several
(more) years for this to land.

However it lands, the interface that it used for NFSv3 should be the
same as the interface that is used for NFSv4 and I think
/proc/fs/nfsd/unlock_filesystem should be one such interface until (if
ever) we discard the /proc/fs/nfsd filesystem.

Thanks,
NeilBrown

>
>
> > For NFSv4.1 and later code is straight forward. We add new state types
> > for admin-revoked state (open, lock, deleg) and change the type of any
> > state on a filesystem - inavlidating any access and closing files as we
> > go. While there are any revoked states we report this to the client in
> > the response to SEQUENCE requests, and it will check and free any states
> > that need to be freed.
> >
> > For NFSv4.0 it isn't quite so easy as there is no mechanism for the
> > client to explicitly acknowledged admin-revoked states. The approach
> > this patchset takes is to discard NFSv4.0 admin-revoked states one
> > lease-time after they were revoked, or immediately for a state that the
> > client tryies to use and gets an "ADMIN_REVOKED" error for. If the
> > filestystem has been unmounted (as expected), the client will see STATE
> > errors before it has a chance to see ADMIN_REVOKED errors, so most often
> > the timeout will be how states are discarded.
> >
> > NeilBrown
> >
> > [PATCH 1/6] nfsd: prepare for supporting admin-revocation of state
> > [PATCH 2/6] nfsd: allow admin-revoked state to appear in
> > [PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.
> > [PATCH 4/6] nfsd: allow lock state ids to be revoked and then freed
> > [PATCH 5/6] nfsd: allow open state ids to be revoked and then freed
> > [PATCH 6/6] nfsd: allow delegation state ids to be revoked and then
> >
>
> --
> Chuck Lever
>

2023-10-27 23:37:53

by Chuck Lever III

[permalink] [raw]

Subject: Re: [PATCH 0/6] support admin-revocation of v4 state

On Sat, Oct 28, 2023 at 09:10:19AM +1100, NeilBrown wrote:
> On Sat, 28 Oct 2023, Chuck Lever wrote:
> > On Fri, Oct 27, 2023 at 12:45:28PM +1100, NeilBrown wrote:
> > > This is a revised version of a patch set I sent over a year ago.
> > > It now supports v4.0 and has had more testing.
> > >
> > > There are cirsumstances where an admin might need to unmount a
> > > filesystem that is NFS-exported and in active use, but does not want to
> > > stop the NFS server completely. These are certainly unusual
> > > circumstance and doing this might negatively impact any clients acting
> > > on the filesystem, but the admin should be able to do this.
> > >
> > > Currently this is quite possible for for NFSv3. Unexporting the
> > > filesystem will ensure no new opens happen, and writing the path name to
> > > /proc/fs/nfsd/unlock_filesystem will ensure anly NLM locks held in the
> > > filesystem are released so that NFSD no longer prevents the filesystem
> > > from being unlocked.
> > >
> > > It is not currently possible for NFSv4. Writing to unlock_filesystem
> > > does not affect NFSv4, which is arguably a bug. This series fixes the bug.
> >
> > I agree that this is a good thing to do.
> >
> > However, I'd like to migrate the "unlock_filesystem" functionality
> > to the nfsd netlink protocol first rather than adding this support
> > to /proc/fs/nfsd/. I don't believe that would be a difficult pre-
> > requisite to get through.
> >
> > Does that seem sensible?
>
> Not to me.
>
> This is not new functionality - it is a fix for existing functionality
> which incorrectly ignores NFSv4.

It's arguable whether this counts as new functionality. I can see
both sides of that argument. For now let's agree to call it a fix,
and let's say it would therefore be appropriate for, say, an -rc
pull request (although I think it will land in nfsd-next for v6.8).

So, on first read through these patches, for some reason I had the
impression that you were adding another file under /proc/fs/nfsd.
That's what I would like to avoid at this point.

Changing the values that can appear in one of these files is an
area that is somewhat more grey. I'm willing to be flexible about
that for now.

> When you say "migrate" I hope you mean to add the "unlock_filesystem"
> functionality to netlink, but not remove it from /proc/fs/nfsd for
> several years at least. I certainly wouldn't want to wait several
> (more) years for this to land.

Naturally. We are playing by the usual rules about kernel-user space
API deprecation. But there's always a fine line about whether an
about-to-be-deprecated API should get new functionality or even bug
fixes.

> However it lands, the interface that it used for NFSv3 should be the
> same as the interface that is used for NFSv4 and I think
> /proc/fs/nfsd/unlock_filesystem should be one such interface until (if
> ever) we discard the /proc/fs/nfsd filesystem.

Fair enough. It wasn't clear to me before that the new state types
described below will not be exposed through the existing
unlock_filesystem interface.

If Jeff agrees, I can pull this into v6.8.

> > > For NFSv4.1 and later code is straight forward. We add new state types
> > > for admin-revoked state (open, lock, deleg) and change the type of any
> > > state on a filesystem - inavlidating any access and closing files as we
> > > go. While there are any revoked states we report this to the client in
> > > the response to SEQUENCE requests, and it will check and free any states
> > > that need to be freed.
> > >
> > > For NFSv4.0 it isn't quite so easy as there is no mechanism for the
> > > client to explicitly acknowledged admin-revoked states. The approach
> > > this patchset takes is to discard NFSv4.0 admin-revoked states one
> > > lease-time after they were revoked, or immediately for a state that the
> > > client tryies to use and gets an "ADMIN_REVOKED" error for. If the
> > > filestystem has been unmounted (as expected), the client will see STATE
> > > errors before it has a chance to see ADMIN_REVOKED errors, so most often
> > > the timeout will be how states are discarded.
> > >
> > > NeilBrown
> > >
> > > [PATCH 1/6] nfsd: prepare for supporting admin-revocation of state
> > > [PATCH 2/6] nfsd: allow admin-revoked state to appear in
> > > [PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.
> > > [PATCH 4/6] nfsd: allow lock state ids to be revoked and then freed
> > > [PATCH 5/6] nfsd: allow open state ids to be revoked and then freed
> > > [PATCH 6/6] nfsd: allow delegation state ids to be revoked and then
> > >
> >
> > --
> > Chuck Lever
> >
>

--
Chuck Lever

2023-10-31 00:14:30

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.

Hi NeilBrown,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6 next-20231030]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/NeilBrown/nfsd-prepare-for-supporting-admin-revocation-of-state/20231027-095832
base: linus/master
patch link: https://lore.kernel.org/r/20231027015613.26247-4-neilb%40suse.de
patch subject: [PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20231031/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231031/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from <command-line>:
fs/nfsd/nfs4state.c: In function 'nfsd4_revoke_states':
>> include/linux/compiler_types.h:425:45: error: call to '__compiletime_assert_955' declared with attribute error: Need native word sized stores/loads for atomicity.
425 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:406:25: note: in definition of macro '__compiletime_assert'
406 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:425:9: note: in expansion of macro '_compiletime_assert'
425 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:428:9: note: in expansion of macro 'compiletime_assert'
428 | compiletime_assert(__native_word(t), \
| ^~~~~~~~~~~~~~~~~~
arch/x86/include/asm/barrier.h:65:9: note: in expansion of macro 'compiletime_assert_atomic_type'
65 | compiletime_assert_atomic_type(*p); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/barrier.h:172:55: note: in expansion of macro '__smp_store_release'
172 | #define smp_store_release(p, v) do { kcsan_release(); __smp_store_release(p, v); } while (0)
| ^~~~~~~~~~~~~~~~~~~
fs/nfsd/nfs4state.c:1725:41: note: in expansion of macro 'smp_store_release'
1725 | smp_store_release(&nn->nfs40_last_revoke,
| ^~~~~~~~~~~~~~~~~
In function 'nfs40_clean_admin_revoked',
inlined from 'nfs4_laundromat' at fs/nfsd/nfs4state.c:6304:2:
include/linux/compiler_types.h:425:45: error: call to '__compiletime_assert_989' declared with attribute error: Need native word sized stores/loads for atomicity.
425 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:406:25: note: in definition of macro '__compiletime_assert'
406 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:425:9: note: in expansion of macro '_compiletime_assert'
425 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:428:9: note: in expansion of macro 'compiletime_assert'
428 | compiletime_assert(__native_word(t), \
| ^~~~~~~~~~~~~~~~~~
arch/x86/include/asm/barrier.h:73:9: note: in expansion of macro 'compiletime_assert_atomic_type'
73 | compiletime_assert_atomic_type(*p); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/barrier.h:176:29: note: in expansion of macro '__smp_load_acquire'
176 | #define smp_load_acquire(p) __smp_load_acquire(p)
| ^~~~~~~~~~~~~~~~~~
fs/nfsd/nfs4state.c:6242:13: note: in expansion of macro 'smp_load_acquire'
6242 | if (smp_load_acquire(&nn->nfs40_last_revoke) == 0 ||
| ^~~~~~~~~~~~~~~~

vim +/__compiletime_assert_955 +425 include/linux/compiler_types.h

eb5c2d4b45e3d2 Will Deacon 2020-07-21 411
eb5c2d4b45e3d2 Will Deacon 2020-07-21 412 #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 413 __compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 414
eb5c2d4b45e3d2 Will Deacon 2020-07-21 415 /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21 416 * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 417 * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21 418 * @msg: a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 419 *
eb5c2d4b45e3d2 Will Deacon 2020-07-21 420 * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 421 * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 422 * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21 423 */
eb5c2d4b45e3d2 Will Deacon 2020-07-21 424 #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @425 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 426

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-10-31 02:46:46

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 1/6] nfsd: prepare for supporting admin-revocation of state

2023-10-31 03:18:09

by kernel test robot

[permalink] [raw]