Received-SPF: pass (google.com: domain of linux-nfs+bounces-77-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sun, 26 Nov 2023 12:36:22 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: NeilBrown <neilb@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>, linux-nfs@vger.kernel.org,
        Olga Kornievskaia <kolga@netapp.com>, Dai Ngo <Dai.Ngo@oracle.com>,
        Tom Talpey <tom@talpey.com>
Subject: Re: [PATCH 05/11] nfsd: prepare for supporting admin-revocation of
 state
Message-ID: <ZWOCFjELHrc7sTil@tissot.1015granger.net>
References: <20231124002925.1816-1-neilb@suse.de>
 <20231124002925.1816-6-neilb@suse.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231124002925.1816-6-neilb@suse.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?jCIWZXKxjHj4OkICXGsMTM/4TBliZrN7LnITdupFfLdsd8SNCSHFMdUkQWMY?=
 =?us-ascii?Q?YgyxX1ZYWuNND2A0bwmrbOcQpLbh/fNJuuMKnni9A3s1U8cY5zkLrjUW0/Sl?=
 =?us-ascii?Q?cDwNCUXVVaRXRBY59poMf24HlzY6sIZv9gUASa462nxIsEDcyo0WqhaNWpEh?=
 =?us-ascii?Q?ySDAULqL1QARpeLL/nNZB15rCYzPzjwZzLKU/51JVmJK0LqKR6CgFOKamovM?=
 =?us-ascii?Q?0PTl+zv3iJLsH2Damq3rLP9YyZsBI7V9PVBQBIr9nTyQlpIiTS1j1qGM0htR?=
 =?us-ascii?Q?sbpxA9FNM5L9nBBrJffvbI4TS0eJx8TxO+cz4DOI2G/aap0O4M+SJU35bWLe?=
 =?us-ascii?Q?tytDng/Bm2sT8cKiHzaLDvGgaknae1rMWqpOYvX5c+YbzRvBaLPw6pxIcI0W?=
 =?us-ascii?Q?/VExhtcH+Q909kxI953QvPGB1amiK7htGrWuxOXBplgDXmZGWlmRneII3SFk?=
 =?us-ascii?Q?Jl53z5sWLhWbRfWN15TyEC7dANVyj+r334oeCt739npexvAOCFmmQm0BsxCq?=
 =?us-ascii?Q?TDu8jHc0duWCZ4hwB46/uhRGDZ/ZpTrNQVhCQIjkeSv1QZV4tSFBlXLu+b4+?=
 =?us-ascii?Q?EnDmuBWKQeNr4MB0rbxFoWxS5+HIz9cWg4n0DdHn5pc++NAlfZamfQRtA6yM?=
 =?us-ascii?Q?CYjT5z8xupXM4pxsQdId5HOFVIWtVUkPJ5ycJdRqOZnRDSEPZewBteq2HcHd?=
 =?us-ascii?Q?u76pwCLZZEaNd4v1DplqaPT730ZUgOObgInPgpkyZHzPl4zmzDBF2AuYxEwV?=
 =?us-ascii?Q?BlJCWgRn+9QOBWj3Nha0jD47ZLZWPVmlStiOU5UWpzssGy3W/LUVVE81/Ind?=
 =?us-ascii?Q?3FjPmrO+qCehOymlr/6FH35DegqwUlMdJ8Z+TWEM+VU1QKfjkpygvoYuUjLD?=
 =?us-ascii?Q?6jpc40KmNWSw5JNvQW8zHkX59CFBXun4j+9NkoP+x2qpX4WV7ZLt/v3rTpN4?=
 =?us-ascii?Q?4WSzU9ohN4Iv6L3+FyeLCOAYp1HatgrNmYGYsjc17rZ17NqRC8a88wFD4ae7?=
 =?us-ascii?Q?rUuTidDLQ7YuP52oSJhpFaJzxqQsuKpn1i0ACfkULudUvbioEZ78n9fRG545?=
 =?us-ascii?Q?CXWe5YF/WumciEyCHyLmrc3Q8hEXC/ZfSBdv820vvHlPpRauhu+eirCNIXho?=
 =?us-ascii?Q?YEZqpYrNmdzE0JD9/ijdo+b3evJuLsS/PXdVOIvfb7KvxQ4Y5mj65w+ki6oZ?=
 =?us-ascii?Q?8VY9Y3IZq0NC7yKuLyODdYlS2sa6KZvMSO7XHOSobjwW6DwQAzgaUoFP+GFo?=
 =?us-ascii?Q?MmgQTGpsHC7qDgvfUXtbeo1dok91UJvy4s4Gupy33Kj08rHtcBw7SyCdp7Rl?=
 =?us-ascii?Q?wU6ULvzdKgFlRt3I/NWXyJWhKOM15AjRRiWuMIVA4gUf+YmfQphX4mMP3G9v?=
 =?us-ascii?Q?05LPx5mBAzRqOD1Y6jWU6NCaaeBB/+9Ntys5oUkfc31Ol3PD7XUNxAhEpx5Y?=
 =?us-ascii?Q?QhH+hBg7eSrMSxcr/iy/nLjSuuCUlfq3Qfb+E4HofNPohWUe3BLkZbkOpBCE?=
 =?us-ascii?Q?W1ZgoZ1K+R3k94lLimGMQD5MEZysyfVrJlrfXYCUMzWCWbXfd9o+3Rh9GO2q?=
 =?us-ascii?Q?reAWH0b0uxQGAo4uZ5GsHDYQeNYV50MDkq03gIp7+E/MRdFk4C3GsQ0WcmkD?=
 =?us-ascii?Q?eQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	KLpBIb25sP4lrDPH3Q8kLbPvuJOCAx8rAwNSSB2lWUu5D+HUuuhdIA7kk7pXiba2BZLW5QrG6QL31lj6mkqoqOt/4vVh5rOO2kryEZJTmBBalqId8SoiT00XN0s32JS8UI5qS9gxxC2TkaZh+zoBiRMDjCbOA55pBTjimWJr06Q+LUwcQkjf4TBR2NdIByyY/HzAeVpqrHuk8hmeXG6MjBDtsSYQNI3aMZC3P5uwAYttNXW0gsI5Qjsm1OqlNRhzcBrYFOBjZalpyTOjdpPsfGbcNW8rNlQdJ4JF6zmIjTQ4+mG0s6GklHlbi8pVgtsLelmzDABjbmuCd4m2/VvqGyZzOiWwQSwE8dx5DPyfcEjA3L6x6+FzN/8618CL/p+ZAUlZoIKyCkGLTrpdJZ+49B7tAakrPYFpysu9idvF/j/E0+tgXIvs5dLFyPVrZ+472ysLkCjExFxgLe+G8KjPPI+gcUbumn7HjqBedTmKLI7qEdw03O+zNtWvZ/PmgrIw+EEo+xrEnLkXYAI4HFnyYiwGPUffuK8zXdb/+a802kZdUM0NdTAWNr5CN4Q7MxyqO8SbA00Xh4woPUuTwKFbd93h5n4Dvx52xckXNAmxJs1CqELCQAZSqXrSnHt/BXG+eyHKTvCkS7SiOHxauoMBnQrKrWdBlfSZPQoG+I/xiWQZu43s1gbUHSlnkoF1Rvw42M9VHRhqKtW/D2ERt3ztIsw0sBtCl9ULbC76+D2Y8dmjpjaRkz9g7oqkc6hVcPniK1s7PEQvrz7n8/u4WvGe1i0jxJEx+sfVYR4vUn3DgNCMhHfRKp1h7Gms1ys/2iWuuxy62Rd2S9oFirgrinxSsy2gncvCWsgCqStqs9//sDYKt3tPijkb2omGsLNe7oYB
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cd780c0e-bac8-477f-0699-08dbeea636b8
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Nov 2023 17:36:25.4765
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xAsSng0zK50toGDUUZ6eg5jdMteFyE5x+iPIkGvN2q0Gscm9YavF0bRg6zRZlfHFsBCgju//6+x/tIYGqXrQEQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5106
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-11-26_17,2023-11-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 bulkscore=0
 mlxlogscore=523 adultscore=0 malwarescore=0 mlxscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311060000
 definitions=main-2311260130
X-Proofpoint-ORIG-GUID: 713fWefjUVZieYrHalehfkd84qjIGHGN
X-Proofpoint-GUID: 713fWefjUVZieYrHalehfkd84qjIGHGN

On Fri, Nov 24, 2023 at 11:28:40AM +1100, NeilBrown wrote:
> 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 a new state-id status NFS4_STID_ADMIN_REVOKE
>    which can be set on open, lock, or delegation state.
>  - 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.
>    Decrements 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 <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 71 ++++++++++++++++++++++++++++++++++++++++++++-
>  fs/nfsd/nfsctl.c    |  1 +
>  fs/nfsd/nfsd.h      |  1 +
>  fs/nfsd/state.h     | 10 +++++++
>  fs/nfsd/trace.h     |  3 +-
>  5 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b9239f2ebc79..477a9e9aebbd 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1215,6 +1215,8 @@ nfs4_put_stid(struct nfs4_stid *s)
>  		return;
>  	}
>  	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
> +	if (s->sc_status & NFS4_STID_ADMIN_REVOKED)
> +		atomic_dec(&s->sc_client->cl_admin_revoked);
>  	nfs4_free_cpntf_statelist(clp->net, s);
>  	spin_unlock(&clp->cl_lock);
>  	s->sc_free(s);
> @@ -1534,6 +1536,8 @@ static void put_ol_stateid_locked(struct nfs4_ol_stateid *stp,
>  	}
>  
>  	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
> +	if (s->sc_status & NFS4_STID_ADMIN_REVOKED)
> +		atomic_dec(&s->sc_client->cl_admin_revoked);
>  	list_add(&stp->st_locks, reaplist);
>  }
>  
> @@ -1679,6 +1683,54 @@ 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 int 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_status == 0 &&
> +		    stid->sc_file->fi_inode->i_sb == sb) {
> +			refcount_inc(&stid->sc_count);
> +			break;
> +		}
> +	spin_unlock(&clp->cl_lock);
> +	return stid;
> +}
> +

nfsd4_revoke_states() needs a kdoc comment.


> +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 int 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) {

This is "dead" code, for now. Does this stub really need to be
introduced in this patch?


> +				}
> +				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)
>  {
> @@ -2550,6 +2602,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;
> @@ -4109,6 +4163,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);
> @@ -4597,7 +4653,9 @@ nfsd4_verify_open_stid(struct nfs4_stid *s)
>  {
>  	__be32 ret = nfs_ok;
>  
> -	if (s->sc_status & NFS4_STID_REVOKED)
> +	if (s->sc_status & NFS4_STID_ADMIN_REVOKED)
> +		ret = nfserr_admin_revoked;
> +	else if (s->sc_status & NFS4_STID_REVOKED)
>  		ret = nfserr_deleg_revoked;
>  	else if (s->sc_status & NFS4_STID_CLOSED)
>  		ret = nfserr_bad_stateid;
> @@ -5188,6 +5246,11 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
>  	deleg = find_deleg_stateid(cl, &open->op_delegate_stateid);
>  	if (deleg == NULL)
>  		goto out;
> +	if (deleg->dl_stid.sc_status & NFS4_STID_ADMIN_REVOKED) {
> +		nfs4_put_stid(&deleg->dl_stid);
> +		status = nfserr_admin_revoked;
> +		goto out;
> +	}
>  	if (deleg->dl_stid.sc_status & NFS4_STID_REVOKED) {
>  		nfs4_put_stid(&deleg->dl_stid);
>  		status = nfserr_deleg_revoked;
> @@ -6508,6 +6571,8 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		 */
>  		statusmask |= NFS4_STID_REVOKED;
>  
> +	statusmask |= NFS4_STID_ADMIN_REVOKED;
> +
>  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
>  		CLOSE_STATEID(stateid))
>  		return nfserr_bad_stateid;
> @@ -6526,6 +6591,10 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		nfs4_put_stid(stid);
>  		return nfserr_deleg_revoked;
>  	}
> +	if (stid->sc_type & NFS4_STID_ADMIN_REVOKED) {
> +		nfs4_put_stid(stid);
> +		return nfserr_admin_revoked;
> +	}
>  	*s = stid;
>  	return nfs_ok;
>  }
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index d6eeee149370..a622d773f428 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -285,6 +285,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 bb00dcd4c1ba..584378c43e0a 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -112,6 +112,7 @@ struct nfs4_stid {
>  #define NFS4_STID_CLOSED	BIT(0)
>  /* For a deleg stateid kept around only to process free_stateid's: */
>  #define NFS4_STID_REVOKED	BIT(1)
> +#define NFS4_STID_ADMIN_REVOKED	BIT(2)

The names of these mask bits are now getting to be visually
indistinguishable from the stateid type names. The subtlety of
where the _STID_ falls in the name makes me blink a few times when
reading this code.

It would be a little more friendly to add _STATUS_ or some other
infix that makes it easy to tell these are not stateid types. I
know that makes the names longer and more unwieldy.


>  	unsigned short		sc_status;
>  
>  	struct list_head	sc_cp_list;
> @@ -388,6 +389,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;
> @@ -752,6 +754,14 @@ static inline void get_nfs4_file(struct nfs4_file *fi)
>  }
>  struct nfsd_file *find_any_file(struct nfs4_file *f);
>  
> +#ifdef CONFIG_NFSD_V4
> +void nfsd4_revoke_states(struct net *net, struct super_block *sb);
> +#else
> +static inline void nfsd4_revoke_states(struct net *net, struct super_block *sb)
> +{
> +}
> +#endif
> +
>  /* grace period management */
>  void nfsd4_end_grace(struct nfsd_net *nn);
>  
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 568b4ec9a2af..281aeb42c9eb 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -651,7 +651,8 @@ DEFINE_STATESEQID_EVENT(open_confirm);
>  #define show_stid_status(x)						\
>  	__print_flags(x, "|",						\
>  		{ NFS4_STID_CLOSED,		"CLOSED" },		\
> -		{ NFS4_STID_REVOKED,		"REVOKED" })		\
> +		{ NFS4_STID_REVOKED,		"REVOKED" },		\
> +		{ NFS4_STID_ADMIN_REVOKED,	"ADMIN_REVOKED" })
>  
>  DECLARE_EVENT_CLASS(nfsd_stid_class,
>  	TP_PROTO(
> -- 
> 2.42.1
> 

-- 
Chuck Lever