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: <4d3e8d4385a511860ec9018b3ca864e7ef3a7b48.camel@hammerspace.com>
Date: Fri, 17 Jan 2025 03:15:16 +0000
From: Trond Myklebust <trondmy@...merspace.com>
To: "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>, "yangerkun@...wei.com" <yangerkun@...wei.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

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.
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.

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