netdev - Re: [PATCH] SUNRPC: Set tk_rpc_status when RPC_TASK

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <fed3cd85-0a15-ae30-b167-84881d6a5efd@huawei.com>
Date: Thu, 16 Jan 2025 19:43:08 +0800
From: yangerkun <yangerkun@...wei.com>
To: Li Lingfeng <lilingfeng3@...wei.com>, <trondmy@...nel.org>,
	<anna@...nel.org>, <chuck.lever@...cle.com>, <jlayton@...nel.org>,
	<neilb@...e.de>, <okorniev@...hat.com>, <Dai.Ngo@...cle.com>,
	<tom@...pey.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <horms@...nel.org>
CC: <linux-nfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<netdev@...r.kernel.org>, <yukuai1@...weicloud.com>, <houtao1@...wei.com>,
	<yi.zhang@...wei.com>, <lilingfeng@...weicloud.com>
Subject: Re: [PATCH] SUNRPC: Set tk_rpc_status when RPC_TASK_SIGNALLED is
 detected

Hi,

Thanks for the patch.

Before 39494194f93b("SUNRPC: Fix races with rpc_killall_tasks()", every
time we set RPC_TASK_SIGNALLED, when we go through __rpc_execute, this
rpc_task will immediate break and exist.

However after that, __rpc_execute won't judge RPC_TASK_SIGNNALED, so for
the case like you point out below, even after your commit
rpc_check_timeout will help break and exist eventually, but this
rpc_task has already do some work. I prefer reintroduce judging
RPC_TASK_SIGNNALED in __rpc_execute to help exist immediatly.

在 2025/1/14 22:41, Li Lingfeng 写道:
> Commit 39494194f93b("SUNRPC: Fix races with rpc_killall_tasks()") adds
> rpc_task_set_rpc_status before setting RPC_TASK_SIGNALLED in
> rpc_signal_task, ensuring that rpc_status is definitely set when
> RPC_TASK_SIGNALLED is set.
> Therefore, it seems unnecessary to set rpc_status again after detecting
> RPC_TASK_SIGNALLED in rpc_check_timeout.
> 
> However, in some exceptional cases, invalid and endlessly looping
> rpc_tasks may be generated. The rpc_call_rpcerror in rpc_check_timeout can
> timely terminate such rpc_tasks. Removing rpc_call_rpcerror may cause the
> rpc_task to fall into an infinite loop.
> 
> For example, in the following situation:
> nfs4_close_done
>   // get err from server
>   nfs4_async_handle_exception
>   // goto out_restart
>                              // umount -f
>                              nfs_umount_begin
>                               rpc_killall_tasks
>                                rpc_signal_task
>                                 rpc_task_set_rpc_status
>                                  task->tk_rpc_status = -ERESTARTSYS
>                                  set_bit
>                                  // set RPC_TASK_SIGNALLED to tk_runstate
>   rpc_restart_call_prepare
>    __rpc_restart_call
>     task->tk_rpc_status = 0
>     // clear tk_rpc_status
>    ...
>    rpc_prepare_task
>     nfs4_close_prepare
>      nfs4_setup_sequence
>       rpc_call_start
>        task->tk_action = call_start
> 
> At this point, an rpc_task with RPC_TASK_SIGNALLED set but tk_rpc_status
> as 0 will be generated. This rpc_task will fall into the following loop:
> call_encode --> call_transmit --> call_transmit_status --> call_status
> --> call_encode.
> 
> Since RPC_TASK_SIGNALLED is set, no request will be sent in call_transmit.
> Similarly, since RPC_TASK_SIGNALLED is set, rq_majortimeo will not be
> updated in call_status --> rpc_check_timeout, which will cause -ETIMEDOUT
> to be directly set to tk_status in call_transmit_status -->
> xprt_request_wait_receive --> xprt_wait_for_reply_request_def -->
> rpc_sleep_on_timeout --> __rpc_sleep_on_priority_timeout.
> 
> Here is the state and loop process of the rpc_task:
> tk_runstate:
> RPC_TASK_RUNNING RPC_TASK_ACTIVE RPC_TASK_NEED_RECV RPC_TASK_SIGNALLED
> tk_xprt->state:
> XPRT_CONNECTED XPRT_BOUND
> tk_flags
> RPC_TASK_ASYNC RPC_TASK_MOVEABLE RPC_TASK_DYNAMIC RPC_TASK_SOFT
> RPC_TASK_NO_RETRANS_TIMEOUT RPC_TASK_CRED_NOREF
> 
> call_encode
>   xprt_request_enqueue_transmit
>    set_bit // RPC_TASK_NEED_XMIT
>   task->tk_action = call_transmit
> 
> call_transmit
>   task->tk_action = call_transmit_status
>   xprt_transmit
>    xprt_request_transmit
>     // check RPC_TASK_SIGNALLED and goto out_dequeue
>     xprt_request_dequeue_transmit
>      xprt_request_dequeue_transmit_locked
>       test_and_clear_bit // RPC_TASK_NEED_XMIT
> 
> call_transmit_status
>   task->tk_action = call_status
>   xprt_request_wait_receive
>    xprt_wait_for_reply_request_def
>     xprt_request_timeout // get timeout
>      req->rq_majortimeo // rq_majortimeo will not be updated
>     rpc_sleep_on_timeout
>      __rpc_sleep_on_priority_timeout
>       task->tk_status = -ETIMEDOUT // set ETIMEDOUT
> 
> call_status
>   task->tk_action = call_encode
>   rpc_check_timeout
>    // check RPC_TASK_SIGNALLED and skip xprt_reset_majortimeo
> 
> Fix it by adding rpc_call_rpcerror back.
> 
> Fixes: 39494194f93b ("SUNRPC: Fix races with rpc_killall_tasks()")
> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
> ---
>   net/sunrpc/clnt.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 0090162ee8c3..0acdff19a37c 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2509,8 +2509,10 @@ rpc_check_timeout(struct rpc_task *task)
>   {
>   	struct rpc_clnt	*clnt = task->tk_client;
>   
> -	if (RPC_SIGNALLED(task))
> +	if (RPC_SIGNALLED(task)) {
> +		rpc_call_rpcerror(task, -ERESTARTSYS);
>   		return;
> +	}
>   
>   	if (xprt_adjust_timeout(task->tk_rqstp) == 0)
>   		return;