linux-kernel - Re: [PATCH] NFSv4: Fix deadlock during the running of state manager

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <baa76c7946676fccafb1dbf658905a0ab3e3bdec.camel@hammerspace.com>
Date: Tue, 17 Dec 2024 17:00:03 +0000
From: Trond Myklebust <trondmy@...merspace.com>
To: "anna@...nel.org" <anna@...nel.org>, "lilingfeng3@...wei.com"
	<lilingfeng3@...wei.com>
CC: "houtao1@...wei.com" <houtao1@...wei.com>, "yukuai1@...weicloud.com"
	<yukuai1@...weicloud.com>, "yangerkun@...wei.com" <yangerkun@...wei.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lilingfeng@...weicloud.com" <lilingfeng@...weicloud.com>,
	"jlayton@...nel.org" <jlayton@...nel.org>, "linux-nfs@...r.kernel.org"
	<linux-nfs@...r.kernel.org>, "yi.zhang@...wei.com" <yi.zhang@...wei.com>
Subject: Re: [PATCH] NFSv4: Fix deadlock during the running of state manager

On Fri, 2024-12-13 at 11:59 +0800, Li Lingfeng wrote:
> Unlinking file may cause the following deadlock in state manager:
> [root@...alhost test]# cat /proc/2943/stack
> [<0>] rpc_wait_bit_killable+0x1a/0x90
> [<0>] _nfs4_proc_delegreturn+0x60f/0x760
> [<0>] nfs4_proc_delegreturn+0x13d/0x2a0
> [<0>] nfs_do_return_delegation+0xba/0x110
> [<0>] nfs_end_delegation_return+0x32c/0x620
> [<0>] nfs_complete_unlink+0xc7/0x290
> [<0>] nfs_dentry_iput+0x36/0x50
> [<0>] __dentry_kill+0xaa/0x250
> [<0>] dput.part.0+0x26c/0x4d0
> [<0>] __put_nfs_open_context+0x1d9/0x260
> [<0>] nfs4_open_reclaim+0x77/0xa0
> [<0>] nfs4_do_reclaim+0x385/0xf40
> [<0>] nfs4_state_manager+0x762/0x14e0
> [<0>] nfs4_run_state_manager+0x181/0x330
> [<0>] kthread+0x1a7/0x1f0
> [<0>] ret_from_fork+0x34/0x60
> [<0>] ret_from_fork_asm+0x1a/0x30
> [root@...alhost test]#
> 
> It can be reproduced by following steps:
> 1) client: open file
> 2) client: unlink file
> 3) server: service restart(trigger state manager in client)
> 4) client: close file(in nfs4_open_reclaim, between
> nfs4_do_open_reclaim
> and put_nfs_open_context)
> 
> Since the file has been open, unlinking will just set
> DCACHE_NFSFS_RENAMED
> for the dentry like this:
> nfs_unlink
>  nfs_sillyrename
>   nfs_async_unlink
>    // set DCACHE_NFSFS_RENAMED
> 
> Restarting service will trigger state manager in client.
> (1) NFS4_SLOT_TBL_DRAINING will be set to nfs4_slot_table since
> session
> has been reset.
> (2) DCACHE_NFSFS_RENAMED is detected in nfs_dentry_iput. Therefore,
> nfs_complete_unlink is called to trigger delegation return.
> (3) Due to the slot table being in draining state and sa_privileged
> being
> 0, the delegation return will be queued and wait.
> nfs4_state_manager
>  nfs4_reset_session
>   nfs4_begin_drain_session
>    nfs4_drain_slot_tbl
>     // set NFS4_SLOT_TBL_DRAINING (1)
>  nfs4_do_reclaim
>   nfs4_open_reclaim
>    __put_nfs_open_context
>     __dentry_kill
>      nfs_dentry_iput // check DCACHE_NFSFS_RENAMED (2)
>       nfs_complete_unlink
>        nfs_end_delegation_return
>         nfs_do_return_delegation
>          nfs4_proc_delegreturn
>           _nfs4_proc_delegreturn
>            rpc_run_task
>             ...
>             nfs4_delegreturn_prepare
>              nfs4_setup_sequence
>               nfs4_slot_tbl_draining // check NFS4_SLOT_TBL_DRAINING
>                                      // and sa_privileged is 0 (3)
>                rpc_sleep_on // set queued and add to slot_tbl_waitq
>                 // rpc_task is async and wait in __rpc_execute
>            rpc_wait_for_completion_task
>             __rpc_wait_for_completion_task
>              out_of_line_wait_on_bit
>               rpc_wait_bit_killable // wait for rpc_task to complete
>  <-------- can not get here to wake up rpc_task -------->
>  nfs4_end_drain_session
>   nfs4_end_drain_slot_table
>    nfs41_wake_slot_table
> 
> On the one hand, the state manager is blocked by the unfinished
> delegation
> return. As a result, nfs4_end_drain_session cannot be invoked to
> clear
> NFS4_SLOT_TBL_DRAINING and wake up waiting tasks.
> On the other hand, since NFS4_SLOT_TBL_DRAINING is not cleared,
> delegation return can only wait in the queue, resulting in a
> deadlock.
> 
> Fix it by turning the delegation return into a privileged operation
> for
> the case where the nfs_client is in NFS4CLNT_RECLAIM_REBOOT state.
> 
> Fixes: 977fcc2b0b41 ("NFS: Add a delegation return into
> nfs4_proc_unlink_setup()")
> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
> ---
>  fs/nfs/nfs4proc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 405f17e6e0b4..f3b1f2725c4e 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6878,7 +6878,7 @@ static int _nfs4_proc_delegreturn(struct inode
> *inode, const struct cred *cred,
>  		data->res.sattr_res = true;
>  	}
>  
> -	if (!data->inode)
> +	if (!data->inode || test_bit(NFS4CLNT_RECLAIM_REBOOT,
> &server->nfs_client->cl_state))
>  		nfs4_init_sequence(&data->args.seq_args, &data-
> >res.seq_res, 1,
>  				   1);
>  	else

Rather than make the delegreturn be privileged, it seems better to make
that delegreturn be asynchronous, just like the unlink itself.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@...merspace.com