Re: [PATCH v2 1/1] nfsd: check that server is running in unlock_filesystem

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-12-12 at 11:47 -0500, Olga Kornievskaia wrote:
> If we are trying to unlock the filesystem via an administrative
> interface and nfsd isn't running, it crashes the server.
> 
> [   59.445578] Modules linked in: nfsd nfs_acl lockd grace nfs_localio ext4 crc16 mbcache jbd2 overlay uinput snd_seq_dummy snd_hrtimer qrtr rfkill vfat fat uvcvideo snd_hda_codec_generic videobuf2_vmalloc videobuf2_memops uvc videobuf2_v4l2 videobuf2_common snd_hda_intel snd_intel_dspcfg snd_hda_codec videodev snd_hda_core snd_hwdep mc snd_seq snd_seq_device snd_pcm snd_timer snd soundcore sg loop auth_rpcgss vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vmw_vmci vsock xfs ghash_ce nvme e1000e nvme_core nvme_keyring nvme_auth hkdf sr_mod cdrom vmwgfx drm_ttm_helper ttm 8021q garp stp llc mrp sunrpc dm_mirror dm_region_hash dm_log iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi fuse dm_multipath dm_mod nfnetlink
> [   59.451979] CPU: 4 UID: 0 PID: 5193 Comm: bash Kdump: loaded Tainted: G    B               6.18.0-rc4+ #74 PREEMPT(voluntary)
> [   59.453311] Tainted: [B]=BAD_PAGE
> [   59.453913] Hardware name: VMware, Inc. VMware20,1/VBSA, BIOS VMW201.00V.24006586.BA64.2406042154 06/04/2024
> [   59.454869] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [   59.455463] pc : nfsd4_revoke_states+0x1b4/0x898 [nfsd]
> [   59.456069] lr : nfsd4_revoke_states+0x19c/0x898 [nfsd]
> [   59.456701] sp : ffff80008cd67900
> [   59.457115] x29: ffff80008cd679d0 x28: 1fffe00016a53f84 x27: dfff800000000000
> [   59.458006] x26: 04b800ef00000000 x25: 1fffe00016a53f80 x24: ffff0000a796ea00
> [   59.458872] x23: ffff0000b89d6000 x22: ffff0000b6c36900 x21: ffff0000b6c36580
> [   59.459738] x20: ffff80008cd67990 x19: ffff0000b6c365c0 x18: 0000000000000000
> [   59.460602] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> [   59.461480] x14: 0000000000000000 x13: 0000000000000001 x12: ffff7000119acf13
> [   59.462272] x11: 1ffff000119acf12 x10: ffff7000119acf12 x9 : dfff800000000000
> [   59.463002] x8 : ffff80008cd67810 x7 : 0000000000000000 x6 : 0097001de0000000
> [   59.463732] x5 : 0000000000000004 x4 : ffff0000b5818000 x3 : 04b800ef00000004
> [   59.464368] x2 : 0000000000000000 x1 : 0000000000000005 x0 : 04b800ef00000000
> [   59.465072] Call trace:
> [   59.465308]  nfsd4_revoke_states+0x1b4/0x898 [nfsd] (P)
> [   59.465830]  write_unlock_fs+0x258/0x440 [nfsd]
> [   59.466278]  nfsctl_transaction_write+0xb0/0x120 [nfsd]
> [   59.466780]  vfs_write+0x1f0/0x938
> [   59.467088]  ksys_write+0xfc/0x1f8
> [   59.467395]  __arm64_sys_write+0x74/0xb8
> [   59.467746]  invoke_syscall.constprop.0+0xdc/0x1e8
> [   59.468177]  do_el0_svc+0x154/0x1d8
> [   59.468489]  el0_svc+0x40/0xe0
> [   59.468767]  el0t_64_sync_handler+0xa0/0xe8
> [   59.469138]  el0t_64_sync+0x1ac/0x1b0
> [   59.469472] Code: 91001343 92400865 d343fc66 110004a1 (38fb68c0)
> [   59.470012] SMP: stopping secondary CPUs
> 
> -- v2 changes to address Neil's comments/suggestions
> changing nfsd4_revoke_states() to take in nfsd_net
> holding nfsd_mutex over nfsd4_revoke_states (making sure to unlock
> and cleanup before return)

Maybe something like this for the description?

"Ensure this can't happen by taking the nfsd_mutex and checking that
the server is still up, and then holding the mutex across the call to
nfsd4_revoke_states()."

> 
> Fixes: 1ac3629bf0125 ("nfsd: prepare for supporting admin-revocation of state")
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  fs/nfsd/nfs4state.c |  3 +--
>  fs/nfsd/nfsctl.c    | 11 ++++++++++-
>  fs/nfsd/state.h     |  4 ++--
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 35004568d43e..191d67973e31 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1769,9 +1769,8 @@ static struct nfs4_stid *find_one_sb_stid(struct nfs4_client *clp,
>   * The clients which own the states will subsequently being notified that the
>   * states have been "admin-revoked".
>   */
> -void nfsd4_revoke_states(struct net *net, struct super_block *sb)
> +void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
>  {
> -	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	unsigned int idhashval;
>  	unsigned int sc_types;
>  
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 2b79129703d5..35bb94f49392 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -259,6 +259,7 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
>  	struct path path;
>  	char *fo_path;
>  	int error;
> +	struct nfsd_net *nn;
>  
>  	/* sanity check */
>  	if (size == 0)
> @@ -285,7 +286,15 @@ 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);
> +	mutex_lock(&nfsd_mutex);

I agree with Neil that having to hold the mutex here is rather icky,
but it should be safe, and since this is done in the context of
administrative controls, performance is less important.

> +	nn = net_generic(netns(file), nfsd_net_id);
> +	if (!nn->nfsd_serv) {
> +		error = -EINVAL;
> +		goto out;
> +	}
> +	nfsd4_revoke_states(nn, path.dentry->d_sb);
> +out:

nit: you can eliminate the goto here by adding an else branch to the
above if statement.

> +	mutex_unlock(&nfsd_mutex);
>  
>  	path_put(&path);
>  	return error;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 1e736f402426..bf3394a01375 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -841,9 +841,9 @@ 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);
> +void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb);
>  #else
> -static inline void nfsd4_revoke_states(struct net *net, struct super_block *sb)
> +static inline void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
>  {
>  }
>  #endif

The approach looks fine though.

Reviewed-by: Jeff Layton <jlayton@kernel.org>