Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
Please, ignore 08.11.2017 15:07, Stanislav Kinsburskiy пишет: > From: Stanislav Kinsburskiy> > Here is the race: > > CPU #0CPU#1 > > cleanup_mnt nfs41_callback_svc (get xprt 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) > > 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. > 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 > > v4: use another mutex to protect callback execution agains per-net transports > shutdown. > This guarantees, that transports won't be destroyed by shutdown callback while > execution is in progress and vice versa. > > v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with > mutex taken), while thread wait for the mutex to take. > The idea is to simply check if thread has to exit, if mutex lock has failed. > This is a busy loop, but it shouldn't happend often and for long. > > Signed-off-by: Stanislav Kinsburskiy > --- > fs/nfs/callback.c | 16 > 1 file changed, 16 insertions(+) > > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c > index 0beb275..4d7979b 100644 > --- a/fs/nfs/callback.c > +++ b/fs/nfs/callback.c > @@ -99,6 +99,8 @@ nfs4_callback_up(struct svc_serv *serv) > } > > #if defined(CONFIG_NFS_V4_1) > +static DEFINE_MUTEX(nfs41_callback_mutex); > + > /* > * The callback service for NFSv4.1 callbacks > */ > @@ -117,6 +119,12 @@ nfs41_callback_svc(void *vrqstp) > if (try_to_freeze()) > continue; > > + mutex_lock(_callback_mutex); > + if (kthread_should_stop()) { > + mutex_unlock(_callback_mutex); > + return 0; > + } > + > prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE); > spin_lock_bh(>sv_cb_lock); > if (!list_empty(>sv_cb_list)) { > @@ -129,8 +137,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(_callback_mutex); > } else { > spin_unlock_bh(>sv_cb_lock); > + mutex_unlock(_callback_mutex); > schedule(); > finish_wait(>sv_cb_waitq, ); > } > @@ -377,7 +387,13 @@ void nfs_callback_down(int minorversion, struct net *net) > struct nfs_callback_data *cb_info = _callback_info[minorversion]; > > mutex_lock(_callback_mutex); > +#if defined(CONFIG_NFS_V4_1) > + mutex_lock(_callback_mutex); > + nfs_callback_down_net(minorversion, cb_info->serv, net); > + mutex_unlock(_callback_mutex); > +#else > nfs_callback_down_net(minorversion, cb_info->serv, net); > +#endif > cb_info->users--; > if (cb_info->users == 0 && cb_info->task != NULL) { > kthread_stop(cb_info->task); > > ___ > Devel mailing list > Devel@openvz.org > https://lists.openvz.org/mailman/listinfo/devel > ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
[Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
From: Stanislav KinsburskiyHere is the race: CPU #0 CPU#1 cleanup_mnt nfs41_callback_svc (get xprt from the list) nfs_callback_down ... ... ... svc_close_net ... ... ... svc_xprt_free ... svc_bc_sock_freebc_svc_process kfree(xprt) svc_process_common rqstp->rq_xprt->xpt_ops (use after free) 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. 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 v4: use another mutex to protect callback execution agains per-net transports shutdown. This guarantees, that transports won't be destroyed by shutdown callback while execution is in progress and vice versa. v3: Fix mutex deadlock, when shutdown callback waits for thread to exit (with mutex taken), while thread wait for the mutex to take. The idea is to simply check if thread has to exit, if mutex lock has failed. This is a busy loop, but it shouldn't happend often and for long. Signed-off-by: Stanislav Kinsburskiy --- fs/nfs/callback.c | 16 1 file changed, 16 insertions(+) diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index 0beb275..4d7979b 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -99,6 +99,8 @@ nfs4_callback_up(struct svc_serv *serv) } #if defined(CONFIG_NFS_V4_1) +static DEFINE_MUTEX(nfs41_callback_mutex); + /* * The callback service for NFSv4.1 callbacks */ @@ -117,6 +119,12 @@ nfs41_callback_svc(void *vrqstp) if (try_to_freeze()) continue; + mutex_lock(_callback_mutex); + if (kthread_should_stop()) { + mutex_unlock(_callback_mutex); + return 0; + } + prepare_to_wait(>sv_cb_waitq, , TASK_INTERRUPTIBLE); spin_lock_bh(>sv_cb_lock); if (!list_empty(>sv_cb_list)) { @@ -129,8 +137,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(_callback_mutex); } else { spin_unlock_bh(>sv_cb_lock); + mutex_unlock(_callback_mutex); schedule(); finish_wait(>sv_cb_waitq, ); } @@ -377,7 +387,13 @@ void nfs_callback_down(int minorversion, struct net *net) struct nfs_callback_data *cb_info = _callback_info[minorversion]; mutex_lock(_callback_mutex); +#if defined(CONFIG_NFS_V4_1) + mutex_lock(_callback_mutex); + nfs_callback_down_net(minorversion, cb_info->serv, net); + mutex_unlock(_callback_mutex); +#else nfs_callback_down_net(minorversion, cb_info->serv, net); +#endif cb_info->users--; if (cb_info->users == 0 && cb_info->task != NULL) { kthread_stop(cb_info->task); ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
07.11.2017 11:03, Kirill Tkhai пишет: >>> Couldn't this change introduce a deadlock like below? >>> [thread] >>> nfs_callback_down() >>> nfs41_callback_svc() >>>mutex_lock(_callback_mutex); >>>kthread_stop(cb_info->task); >>> wake_up_process(); >>> wait_for_completion(>exited); > > And what about above one? > Good catch. Need to think more about it then. ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
On 07.11.2017 13:01, Stanislav Kinsburskiy wrote: > Sure, here it is: > > CPU #0CPU#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>>> >>> 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 >>> --- >>> 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(>sv_cb_waitq, , TASK_INTERRUPTIBLE); >>> + mutex_lock(_callback_mutex); >>> spin_lock_bh(>sv_cb_lock); >>> if (!list_empty(>sv_cb_list)) { >>> req = list_first_entry(>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(_callback_mutex); >>> } else { >>> spin_unlock_bh(>sv_cb_lock); >>> + mutex_unlock(_callback_mutex); >>> schedule(); >>> finish_wait(>sv_cb_waitq, ); >>> } >> >> Couldn't this change introduce a deadlock like below? >> [thread] >> nfs_callback_down() >> nfs41_callback_svc() >>mutex_lock(_callback_mutex); >>kthread_stop(cb_info->task); >> wake_up_process(); >> wait_for_completion(>exited); And what about above one? ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
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_freebc_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>> >> 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 >> --- >> 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(>sv_cb_waitq, , TASK_INTERRUPTIBLE); >> +mutex_lock(_callback_mutex); >> spin_lock_bh(>sv_cb_lock); >> if (!list_empty(>sv_cb_list)) { >> req = list_first_entry(>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(_callback_mutex); >> } else { >> spin_unlock_bh(>sv_cb_lock); >> +mutex_unlock(_callback_mutex); >> schedule(); >> finish_wait(>sv_cb_waitq, ); >> } > > Couldn't this change introduce a deadlock like below? > [thread] > nfs_callback_down() nfs41_callback_svc() >mutex_lock(_callback_mutex); >kthread_stop(cb_info->task); > wake_up_process(); > wait_for_completion(>exited); > > > mutex_lock(_callback_mutex); > > ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
On 03.11.2017 19:47, Stanislav Kinsburskiy wrote: > From: Stanislav Kinsburskiy> > 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 > --- > 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(>sv_cb_waitq, , TASK_INTERRUPTIBLE); > + mutex_lock(_callback_mutex); > spin_lock_bh(>sv_cb_lock); > if (!list_empty(>sv_cb_list)) { > req = list_first_entry(>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(_callback_mutex); > } else { > spin_unlock_bh(>sv_cb_lock); > + mutex_unlock(_callback_mutex); > schedule(); > finish_wait(>sv_cb_waitq, ); > } Couldn't this change introduce a deadlock like below? [thread] nfs_callback_down() nfs41_callback_svc() mutex_lock(_callback_mutex); kthread_stop(cb_info->task); wake_up_process(); wait_for_completion(>exited); mutex_lock(_callback_mutex); ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
Going to send it to mainstream as well? -- Best regards, Konstantin Khorenko, Virtuozzo Linux Kernel Team On 11/03/2017 07:47 PM, Stanislav Kinsburskiy wrote: From: Stanislav KinsburskiyThe 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. 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 --- 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(>sv_cb_waitq, , TASK_INTERRUPTIBLE); + mutex_lock(_callback_mutex); spin_lock_bh(>sv_cb_lock); if (!list_empty(>sv_cb_list)) { req = list_first_entry(>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(_callback_mutex); } else { spin_unlock_bh(>sv_cb_lock); + mutex_unlock(_callback_mutex); schedule(); finish_wait(>sv_cb_waitq, ); } ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel . ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
Well... Maybe. Let's check, how it works with our kernel. 07.11.2017 10:16, Konstantin Khorenko пишет: > Going to send it to mainstream as well? > > -- > Best regards, > > Konstantin Khorenko, > Virtuozzo Linux Kernel Team > > On 11/03/2017 07:47 PM, Stanislav Kinsburskiy wrote: >> From: Stanislav Kinsburskiy>> >> 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. >> 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 >> --- >> 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(>sv_cb_waitq, , TASK_INTERRUPTIBLE); >> + mutex_lock(_callback_mutex); >> spin_lock_bh(>sv_cb_lock); >> if (!list_empty(>sv_cb_list)) { >> req = list_first_entry(>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(_callback_mutex); >> } else { >> spin_unlock_bh(>sv_cb_lock); >> + mutex_unlock(_callback_mutex); >> schedule(); >> finish_wait(>sv_cb_waitq, ); >> } >> >> ___ >> Devel mailing list >> Devel@openvz.org >> https://lists.openvz.org/mailman/listinfo/devel >> . >> ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
[Devel] [PATCH] nfs: protect callback execution against per-net callback thread shutdown
From: Stanislav KinsburskiyThe 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. 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 --- 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(>sv_cb_waitq, , TASK_INTERRUPTIBLE); + mutex_lock(_callback_mutex); spin_lock_bh(>sv_cb_lock); if (!list_empty(>sv_cb_list)) { req = list_first_entry(>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(_callback_mutex); } else { spin_unlock_bh(>sv_cb_lock); + mutex_unlock(_callback_mutex); schedule(); finish_wait(>sv_cb_waitq, ); } ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel