linux-kernel - 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] [day] [month] [year] [list]

Message-ID: <3f889f28-907b-67a7-8b12-04e31812daac@huawei.com>
Date: Fri, 17 Jan 2025 20:16:12 +0800
From: yangerkun <yangerkun@...wei.com>
To: Trond Myklebust <trondmy@...merspace.com>, "tom@...pey.com"
	<tom@...pey.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"chuck.lever@...cle.com" <chuck.lever@...cle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "lilingfeng3@...wei.com" <lilingfeng3@...wei.com>,
	"okorniev@...hat.com" <okorniev@...hat.com>, "anna@...nel.org"
	<anna@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"horms@...nel.org" <horms@...nel.org>, "Dai.Ngo@...cle.com"
	<Dai.Ngo@...cle.com>, "jlayton@...nel.org" <jlayton@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "neilb@...e.de" <neilb@...e.de>
CC: "houtao1@...wei.com" <houtao1@...wei.com>, "linux-nfs@...r.kernel.org"
	<linux-nfs@...r.kernel.org>, "yi.zhang@...wei.com" <yi.zhang@...wei.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lilingfeng@...weicloud.com" <lilingfeng@...weicloud.com>,
	"yukuai1@...weicloud.com" <yukuai1@...weicloud.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: Re: [PATCH] SUNRPC: Set tk_rpc_status when RPC_TASK_SIGNALLED is
 detected



在 2025/1/17 11:15, Trond Myklebust 写道:
> On Fri, 2025-01-17 at 10:29 +0800, yangerkun wrote:
>>
>>
>> 在 2025/1/17 4:52, Trond Myklebust 写道:
>>> On Thu, 2025-01-16 at 19:43 +0800, yangerkun wrote:
>>>> 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.
>>>>
>>>
>>> Better yet... Let's get rid of the RPC_TASK_SIGNALLED flag
>>> altogether
>>> and just replace
>>>
>>> #define RPC_TASK_SIGNALLED(task) (READ_ONCE(task->tk_rpc_status) ==
>>> -ERESTARTSYS)
>>
>> Hi,
>>
>> Thanks for your reply! Yeah, if all the places where tk_rpc_status is
>> updated are by calling rpc_task_set_rpc_status, we can use
>> task->tk_rpc_status == -ERESTARTSYS to determine whether rpc_task is
>> RPC_TASK_SIGNALLED. But for the case like Li has provided,
>> __rpc_restart_call won't do this, and will overwrite tk_rpc_status
>> unconditionally. This won't be a stable solution. Maybe it's better
>> to
>> change __rpc_restart_call calling rpc_task_set_rpc_status too? And
>> __rpc_execute will be enough to help solve this case.
>>
>>
> 
> That would break __rpc_restart_call() to the point of rendering it
> completely useless.

Aha, agree with you. There maybe case tk_rpc_status != 0 but
rpc_restart_call can also rerun this rpc_task.

The only case we should consider is when we call rpc_signal_task, we
should break this rpc_task unconditionally. Fixing it within
__rpc_execute is enough.

> The whole purpose of that call is to give the NFS layer a chance to
> handle errors in the exit callback, and then kick off a fresh call.
> Your suggestion would mean that any RPC level error sticks around, and
> causes the new call to immediately fail.
> 
> I see no point in doing anything more than fixing the looping
> behaviour. Eliminating the redundant flag will do that.
>