[Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown

[Stanislav Kinsburskiy]

Sure, here it is:

CPU #0					CPU#1

cleanup_mnt				nfs41_callback_svc
...						(get transport from the list)
nfs_callback_down			...
...					...
svc_close_net				...
...					...
svc_xprt_free				...
svc_bc_sock_free			bc_svc_process
kfree(xprt)					svc_process_common
						rqstp->rq_xprt->xpt_ops (use after free)
					


07.11.2017 10:49, Kirill Tkhai пишет:
> On 03.11.2017 19:47, Stanislav Kinsburskiy wrote:
>> From: Stanislav Kinsburskiy <skinsbursky at parallels.com>
>>
>> The problem is that per-net SUNRPC transports shutdown is done regardless
>> current callback execution. This is a race leading to transport use-after-free
>> in callback handler.
> 
> Could you please draw the race to show the interaction between functions?
> 
>> This patch fixes it in stright-forward way. I.e. it protects callback
>> execution with the same mutex used for per-net data creation and destruction.
>> Hopefully, it won't slow down NFS client significantly.
>>
>> https://jira.sw.ru/browse/PSBM-75751
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky at parallels.com>
>> ---
>>  fs/nfs/callback.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>> index 0beb275..82e8ed1 100644
>> --- a/fs/nfs/callback.c
>> +++ b/fs/nfs/callback.c
>> @@ -118,6 +118,7 @@ nfs41_callback_svc(void *vrqstp)
>>  			continue;
>>  
>>  		prepare_to_wait(&serv->sv_cb_waitq, &wq, TASK_INTERRUPTIBLE);
>> +		mutex_lock(&nfs_callback_mutex);
>>  		spin_lock_bh(&serv->sv_cb_lock);
>>  		if (!list_empty(&serv->sv_cb_list)) {
>>  			req = list_first_entry(&serv->sv_cb_list,
>> @@ -129,8 +130,10 @@ nfs41_callback_svc(void *vrqstp)
>>  			error = bc_svc_process(serv, req, rqstp);
>>  			dprintk("bc_svc_process() returned w/ error code= %d\n",
>>  				error);
>> +			mutex_unlock(&nfs_callback_mutex);
>>  		} else {
>>  			spin_unlock_bh(&serv->sv_cb_lock);
>> +			mutex_unlock(&nfs_callback_mutex);
>>  			schedule();
>>  			finish_wait(&serv->sv_cb_waitq, &wq);
>>  		}
> 
> Couldn't this change introduce a deadlock like below?
>                                                           [thread]
> nfs_callback_down()                                       nfs41_callback_svc()
>    mutex_lock(&nfs_callback_mutex);                       
>    kthread_stop(cb_info->task);                           
>       wake_up_process();                                  
>       wait_for_completion(&kthread->exited);              
> 
>                                                           mutex_lock(&nfs_callback_mutex); 
> 
>