| 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
| ||
|
Message-ID: <751d31cc-640f-47fc-bb5e-1f9fbdd6316b@huawei.com>
Date: Thu, 16 Jan 2025 22:32:09 +0800
From: Li Lingfeng <lilingfeng3@...wei.com>
To: yangerkun <yangerkun@...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
在 2025/1/16 19:43, yangerkun 写道:
> 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.
Thanks for your advice, I will send v2 soon.
>
> 在 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;
Powered by blists - more mailing lists