<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div dir="auto">
<div><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">19 июля 2017 г. 9:37 PM пользователь Andrey Vagin <avagin@virtuozzo.com> написал:<br type="attribution">
<blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><font size="2"><span style="font-size:10pt">
<div>On Wed, Jul 19, 2017 at 11:31:28AM -0700, Stanislav Kinsburskiy wrote:<br>
> <br>
> <br>
> 19 июля 2017 г. 9:14 PM пользователь Andrey Vagin <avagin@virtuozzo.com><br>
> написал:<br>
> <br>
> On Wed, Jul 19, 2017 at 08:04:22PM +0300, Andrey Ryabinin wrote:<br>
> ><br>
> ><br>
> > On 07/19/2017 04:14 AM, Andrei Vagin wrote:<br>
> > > From: Andrei Vagin <avagin@virtuozzo.com><br>
> > ><br>
> > > task_work_run() has to be called before exit_task_namespaces(),<br>
> > > because fuse_abort_conn() is called from __fput(). If it will not<br>
> > > be executed, we can hang in request_wait_answer(). We have seen this<br>
> > > situation when a process was the last member of a mount namespace<br>
> > > and the mount namespace has a vstorage fuse mount.<br>
> > ><br>
> ><br>
> > Can we pleas have a changelog that doesn't look like an output of random<br>
> text generator?<br>
> <br>
> Thanks!<br>
> <br>
> > The fact that "fuse_abort_conn() is called from __fput()" doesn't really<br>
> explain why<br>
> > task_work_run() needs to be called before exit_task_namespaces.<br>
> ><br>
> <br>
> Here is another version of my random text generator. It has to be more<br>
> detailed.<br>
> <br>
> This patch solves a problem for a following case. We have a container (a<br>
> group of processes in pid and mount namespaces) with a fuse mount. An<br>
> init process exits and the kernel kills all process in its pid<br>
> namespace. There is a fuse daemon, which handle the fuse mount.<br>
> Currently the kernel kills this process and closes all its file<br>
> descriptors, but __fput() for them is postponed and they will be<br>
> called from a task_work. Then the kernel starts destroying the mount<br>
> namespace and the fuse mount, it sees that a control descriptor for<br>
> this mount is alive and sends a request to a fuse daemon:<br>
> <br>
> $ cat /proc/4353/task/4355/stack<br>
> [<ffffffffa04c3451>] request_wait_answer+0x91/0x270 [fuse]<br>
> [<ffffffffa04c36b7>] __fuse_request_send+0x87/0xe0 [fuse]<br>
> [<ffffffffa04c6c47>] fuse_request_check_and_send+0x27/0x30 [fuse]<br>
> [<ffffffffa04c6c60>] fuse_request_send+0x10/0x20 [fuse]<br>
> [<ffffffffa04d2f35>] fuse_put_super+0x55/0xc0 [fuse]<br>
> [<ffffffff81218b32>] generic_shutdown_super+0x72/0xf0<br>
> [<ffffffff81218f12>] kill_anon_super+0x12/0x20<br>
> [<ffffffffa04d2577>] fuse_kill_sb_anon+0x47/0x50 [fuse]<br>
> [<ffffffff812194a9>] deactivate_locked_super+0x49/0x80<br>
> [<ffffffff81219526>] deactivate_super+0x46/0x60<br>
> [<ffffffff81237145>] mntput_no_expire+0xc5/0x120<br>
> [<ffffffff812371c4>] mntput+0x24/0x40<br>
> [<ffffffff812372f8>] namespace_unlock+0x118/0x130<br>
> [<ffffffff81239f2b>] put_mnt_ns+0x4b/0x60<br>
> [<ffffffff810b786b>] free_nsproxy+0x1b/0x90<br>
> [<ffffffff810b7a0a>] switch_task_namespaces+0x5a/0x70<br>
> [<ffffffff810b7ae0>] exit_task_namespaces+0x10/0x20<br>
> [<ffffffff8108c883>] do_exit+0x2f3/0xb20<br>
> [<ffffffff8108d12f>] do_group_exit+0x3f/0xa0<br>
> [<ffffffff8109e760>] get_signal_to_deliver+0x1d0/0x6d0<br>
> [<ffffffff8102a357>] do_signal+0x57/0x6b0<br>
> [<ffffffff8102aa0f>] do_notify_resume+0x5f/0xb0<br>
> [<ffffffff8169273d>] int_signal+0x12/0x17<br>
> [<ffffffffffffffff>] 0xffffffffffffffff<br>
> <br>
> But we know that a fuse daemon is already dead and the control<br>
> descriptor isn't closed completely, because __fput() was postponed.<br>
> <br>
> This patch calls task_work_run() before destroying namespaces to<br>
> complete closing all process files.<br>
> <br>
> <br>
> Now I have a question. Because rised questions look reasonable.<br>
> This sounds like a generic issue.<br>
> I.e. it's either solved in upstream likewise or otherwise. Or not solved at<br>
> all.<br>
> I.e. what's the status of this issue in linux-next?<br>
> <br>
<br>
In the upstream kernel deactivate_super() is called from a task_work too,<br>
so there is not this problem. But we can't backport these changes from<br>
the upstream, because they are too big.<br>
</div>
</span></font></div>
</blockquote>
</div>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">Then probably it worth to mention the upsteam solution in the change log and why we can't use it, and how our patch logic correlates with the upstream one.</div>
<div dir="auto">Hm?</div>
<div dir="auto"><br>
</div>
<div dir="auto">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><font size="2"><span style="font-size:10pt">
<div><br>
> <br>
> <br>
> ><br>
> ><br>
> > > <a href="https://jira.sw.ru/browse/PSBM-68266">https://jira.sw.ru/browse/PSBM-68266</a><br>
> > > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com><br>
> > > ---<br>
> > > include/linux/task_work.h | 9 +++++++--<br>
> > > kernel/exit.c | 9 +++++++++<br>
> > > kernel/task_work.c | 4 ++--<br>
> > > 3 files changed, 18 insertions(+), 4 deletions(-)<br>
> > ><br>
> > > diff --git a/include/linux/task_work.h b/include/linux/task_work.h<br>
> > > index ca5a1cf..b3af76d 100644<br>
> > > --- a/include/linux/task_work.h<br>
> > > +++ b/include/linux/task_work.h<br>
> > > @@ -14,11 +14,16 @@ init_task_work(struct callback_head *twork,<br>
> task_work_func_t func)<br>
> > > <br>
> > > int task_work_add(struct task_struct *task, struct callback_head<br>
> *twork, bool);<br>
> > > struct callback_head *task_work_cancel(struct task_struct *,<br>
> task_work_func_t);<br>
> > > -void task_work_run(void);<br>
> > > +void __task_work_run(bool exiting);<br>
> > > +<br>
> > > +static inline void task_work_run(void)<br>
> > > +{<!-- --><br>
> > > + return __task_work_run(false);<br>
> > > +}<br>
> > > <br>
> > > static inline void exit_task_work(struct task_struct *task)<br>
> > > {<!-- --><br>
> > > - task_work_run();<br>
> > > + __task_work_run(true);<br>
> > > }<br>
> > > <br>
> > > #endif /* _LINUX_TASK_WORK_H */<br>
> > > diff --git a/kernel/exit.c b/kernel/exit.c<br>
> > > index 3c83db2..ea54a73 100644<br>
> > > --- a/kernel/exit.c<br>
> > > +++ b/kernel/exit.c<br>
> > > @@ -827,6 +827,15 @@ void do_exit(long code)<br>
> > > exit_fs(tsk);<br>
> > > if (group_dead)<br>
> > > disassociate_ctty(1);<br>
> > > +<br>
> > > + /*<br>
> > > + * task_work_run() has to be called before exit_task_namespaces(),<br>
> > > + * because fuse_abort_conn() is called from __fput(). If it will<br>
> not<br>
> > > + * be executed, we can hang in request_wait_answer(). We have seen<br>
> this<br>
> > > + * situation when a process was the last member of a mount<br>
> namespace<br>
> > > + * and the mount namespace has a vstorage fuse mount.<br>
> > > + */<br>
> > > + task_work_run();<br>
> ><br>
> > Given that this is purely fuse's problem, maybe request_wait_answer()<br>
> could just call task_work_run()?<br>
> ><br>
> > Or maybe we can just call exit_task_work(tsk) before exit_task_namespaces<br>
> (tsk). This seems fine to me,<br>
> > but perhaps I'm missing something.<br>
> ><br>
> > > exit_task_namespaces(tsk);<br>
> > > exit_task_work(tsk);<br>
> > > <br>
> > > diff --git a/kernel/task_work.c b/kernel/task_work.c<br>
> > > index 65bd3c9..f0000c4 100644<br>
> > > --- a/kernel/task_work.c<br>
> > > +++ b/kernel/task_work.c<br>
> > > @@ -46,7 +46,7 @@ task_work_cancel(struct task_struct *task,<br>
> task_work_func_t func)<br>
> > > return work;<br>
> > > }<br>
> > > <br>
> > > -void task_work_run(void)<br>
> > > +void __task_work_run(bool exiting)<br>
> > > {<!-- --><br>
> > > struct task_struct *task = current;<br>
> > > struct callback_head *work, *head, *next;<br>
> > > @@ -58,7 +58,7 @@ void task_work_run(void)<br>
> > > */<br>
> > > do {<!-- --><br>
> > > work = ACCESS_ONCE(task->task_works);<br>
> > > - head = !work && (task->flags & PF_EXITING) ?<br>
> > > + head = !work && exiting ?<br>
> ><br>
> > Why we need this change? AFAIU this will allow to add more task_works in<br>
> exit_task_namespaces()<br>
> > before final exit_task_work(). What's the point of this?<br>
> ><br>
> > > &work_exited : NULL;<br>
> > > } while (cmpxchg(&task->task_works, work, head) != work);<br>
> > > <br>
> > ><br>
> _______________________________________________<br>
> Devel mailing list<br>
> Devel@openvz.org<br>
> <a href="https://lists.openvz.org/mailman/listinfo/devel">https://lists.openvz.org/mailman/listinfo/devel</a><br>
> <br>
> <br>
</div>
</span></font></div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</body>
</html>