netdev - Re: [PATCH] RPC: killing RPC tasks races fixed

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]

Date:	Thu, 17 Mar 2011 18:43:56 +0300
From:	Stanislav Kinsbursky <skinsbursky@...allels.com>
To:	Trond Myklebust <Trond.Myklebust@...app.com>
CC:	"linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
	Pavel Emelianov <xemul@...allels.com>,
	"neilb@...e.de" <neilb@...e.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"bfields@...ldses.org" <bfields@...ldses.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"skinsbursky@...nvz.org" <skinsbursky@...nvz.org>
Subject: Re: [PATCH] RPC: killing RPC tasks races fixed

17.03.2011 16:01, Trond Myklebust пишет:
> On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:
>> task->tk_waitqueue must be checked for NULL before trying to wake up task in
>> rpc_killall_tasks() because it can be NULL.
>>
>> Here is an example:
>>
>> CPU 0               	CPU 1				CPU 2
>> --------------------	---------------------	--------------------------
>> nfs4_run_open_task
>> rpc_run_task
>> rpc_execute
>> rpc_set_active
>> rpc_make_runnable
>> (waiting)
>> 			rpc_async_schedule
>> 			nfs4_open_prepare
>> 			nfs_wait_on_sequence
>> 						nfs_umount_begin
>> 						rpc_killall_tasks
>> 						rpc_wake_up_task
>> 						rpc_wake_up_queued_task
>> 						spin_lock(tk_waitqueue == NULL)
>> 						BUG()
>> 			rpc_sleep_on
>> 			spin_lock(&q->lock)
>> 			__rpc_sleep_on
>> 			task->tk_waitqueue = q
>>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@...nvz.org>
>>
>> ---
>>   net/sunrpc/clnt.c |    4 +++-
>>   1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 57d344c..24039fe 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
>>   		if (!(rovr->tk_flags&  RPC_TASK_KILLED)) {
>>   			rovr->tk_flags |= RPC_TASK_KILLED;
>>   			rpc_exit(rovr, -EIO);
>> -			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
>> +			if (rovr->tk_waitqueue)
>> +				rpc_wake_up_queued_task(rovr->tk_waitqueue,
>> +							rovr);
>
> Testing for RPC_IS_QUEUED(rovr) would be better, since that would
> optimise away the call to rpc_wake_up_queued_task() altogether for those
> tasks that aren't queued.
>

Yes, I agree with testing RPC_IS_QUEUED(rovr) since such approach looks
clearer and in 2.6.38 tk_waitqueue is initialized prior to set
RPC_TASK_QUEUED bit.
But I found this problem in 2.6.32 rhel kernel where this set sequence is inversed.
Will send fixed version soon.

>>   		}
>>   	}
>>   	spin_unlock(&clnt->cl_lock);
>>
>


-- 
Best regards,
Stanislav Kinsbursky
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html