[PATCH] move exit_task_work() before exit_fs().

[PATCH] move exit_task_work() before exit_fs().

[Leon Ma]

We encountered following panic. The scenario is the process is exiting and executing its
task work. When closing dev node, the driver triggers a firmware reload according to device
status. Because task->fs is set to NULL in exit_fs(), panic happens.
Task work is a common interface, we should not limite the resource the user will utilize.

[  118.521972] task: ffff880038de5cd0 ti: ffff880038e86000 task.ti: ffff880038e86000
[  118.521977] RIP: 0010:[<ffffffff8217a0a6>]  [<ffffffff8217a0a6>] path_init+0x336/0x440
[  118.521986] RSP: 0000:ffff880038e876a0  EFLAGS: 00010246
[  118.521991] RAX: 0000000000000000 RBX: ffff880038e87790 RCX: 0000000000000071
[  118.521996] RDX: ffff88003f30f5d4 RSI: ffffffff82179f75 RDI: ffffffff8209b167
[  118.522000] RBP: ffff880038e876e0 R08: ffff880038e87730 R09: ffff8800218b44c0
[  118.522005] R10: 0000000000000000 R11: 000000000000000f R12: ffff880038ebf000
[  118.522010] R13: ffff880038e87870 R14: ffff880038e87850 R15: 00000000ffffff9c
[  118.522015] FS:  0000000000000000(0000) GS:ffff88003f300000(0000) knlGS:0000000000000000
[  118.522020] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[  118.522025] CR2: 0000000000000020 CR3: 0000000039d4f000 CR4: 00000000001007e0
[  118.522030] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  118.522035] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  118.522039] Stack:
[  118.522043]  ffff880038dbc300 ffff880038e876b8 ffffffff822cf046 ffff88002e434f00
[  118.522056]  ffff880038e87790 ffff880038e87870 ffff880038e87850 00000000ffffff9c
[  118.522069]  ffff880038e87780 ffffffff8217d419 ffff88003b1de480 ffff88003bbf7700
[  118.522082] Call Trace:
[  118.522092]  [<ffffffff822cf046>] ? security_file_alloc+0x16/0x20
[  118.522100]  [<ffffffff8217d419>] path_openat+0x69/0x4b0
[  118.522109]  [<ffffffff826b5c7f>] ? is_connected_output_ep+0x15f/0x260
[  118.522117]  [<ffffffff8217dae9>] do_filp_open+0x39/0x90
[  118.522125]  [<ffffffff823316ca>] ? string.isra.3+0x3a/0xe0
[  118.522133]  [<ffffffff82332809>] ? vsnprintf+0x209/0x620
[  118.522141]  [<ffffffff8216ced2>] file_open_name+0xb2/0xf0
[  118.522148]  [<ffffffff8216cf46>] filp_open+0x36/0x40
[  118.522157]  [<ffffffff82404441>] _request_firmware+0x351/0x9c0
[  118.522164]  [<ffffffff82404b26>] request_firmware+0x16/0x20
[  118.522172]  [<ffffffff826dd733>] sst_request_fw+0x63/0x510
[  118.522180]  [<ffffffff8283be10>] ? __mutex_lock_slowpath+0x280/0x3b0
[  118.522188]  [<ffffffff826df047>] sst_load_fw+0x1f7/0x4a0
[  118.522195]  [<ffffffff826da81f>] sst_download_fw+0xf/0x60
[  118.522202]  [<ffffffff826db022>] intel_sst_check_device+0x92/0x1f0
[  118.522209]  [<ffffffff826db4bd>] sst_set_generic_params+0x18d/0x500
[  118.522218]  [<ffffffff826cee25>] sst_fill_and_send_cmd.constprop.7+0x95/0x130
[  118.522226]  [<ffffffff826cf1dd>] sst_send_gain_cmd+0x9d/0xe0
[  118.522233]  [<ffffffff826cf283>] sst_set_pipe_gain+0x63/0xa0
[  118.522241]  [<ffffffff826d0665>] sst_send_pipe_gains+0xd5/0x2b0
[  118.522248]  [<ffffffff826cb5d6>] sst_media_digital_mute+0x46/0x80
[  118.522257]  [<ffffffff826ab5b1>] snd_soc_dai_digital_mute+0x21/0x60
[  118.522265]  [<ffffffff826bc39d>] soc_pcm_close+0xbd/0x250
[  118.522272]  [<ffffffff826bdf2a>] dpcm_fe_dai_close+0x7a/0x150
[  118.522281]  [<ffffffff8267f138>] snd_pcm_release_substream+0x58/0xb0
[  118.522289]  [<ffffffff8267f1cf>] snd_pcm_release+0x3f/0xa0//////////
[  118.522296]  [<ffffffff8216f600>] __fput+0xf0/0x240
[  118.522304]  [<ffffffff8216f75e>] ____fput+0xe/0x10
[  118.522312]  [<ffffffff82090195>] task_work_run+0xa5/0xd0
[  118.522320]  [<ffffffff82072a08>] do_exit+0x2b8/0xad0
[  118.522327]  [<ffffffff8283d3bf>] ? __schedule+0x3df/0x820
[  118.522335]  [<ffffffff820733af>] do_group_exit+0x3f/0xa0
[  118.522343]  [<ffffffff82082b6e>] get_signal_to_deliver+0x24e/0x650
[  118.522352]  [<ffffffff8200232d>] do_signal+0x4d/0x960
[  118.522360]  [<ffffffff82097584>] ? hrtimer_start_range_ns+0x14/0x20
[  118.522368]  [<ffffffff8262a98d>] ? binder_ioctl+0x15d/0x990
[  118.522377]  [<ffffffff82002ca5>] do_notify_resume+0x65/0x80
[  118.522384]  [<ffffffff82844efa>] int_signal+0x12/0x17

Signed-off-by: Leon Ma <xindong.ma@intel.com>
Signed-off-by: Zhang Di <di.zhang@intel.com>
Signed-off-by: Sun Zhonghua <zhonghua.sun@intel.com>
---
 kernel/exit.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 1ea4369..64ba13b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -729,6 +729,8 @@ void do_exit(long code)
 	tsk->exit_code = code;
 	taskstats_exit(tsk, group_dead);
 
+	exit_files(tsk);
+	exit_task_work(tsk);
 	exit_mm(tsk);
 
 	if (group_dead)
@@ -737,12 +739,10 @@ void do_exit(long code)
 
 	exit_sem(tsk);
 	exit_shm(tsk);
-	exit_files(tsk);
 	exit_fs(tsk);
 	if (group_dead)
 		disassociate_ctty(1);
 	exit_task_namespaces(tsk);
-	exit_task_work(tsk);
 	exit_thread();
 
 	/*
-- 
1.7.9.5

Re: [PATCH] move exit_task_work() before exit_fs().

[Oleg Nesterov]

On 12/26, Leon Ma wrote:
>
> We encountered following panic. The scenario is the process is exiting and executing its
> task work. When closing dev node, the driver triggers a firmware reload according to device
> status. Because task->fs is set to NULL in exit_fs(), panic happens.

I think this should be fixed somewhere else...

> Task work is a common interface, we should not limite the resource the user will utilize.

Exactly. And note that with this patch exit_mm()..disassociate_ctty() paths
can't use task works.

Not to mention that this patch moves exit_files() up, even before exit_mm(),
without any explanation.

Add Al. May be we can move exit_fs() down after exit_task_work(), I dunno,
but to me it would be better to change the driver.


> [  118.521972] task: ffff880038de5cd0 ti: ffff880038e86000 task.ti: ffff880038e86000
> [  118.521977] RIP: 0010:[<ffffffff8217a0a6>]  [<ffffffff8217a0a6>] path_init+0x336/0x440
> [  118.521986] RSP: 0000:ffff880038e876a0  EFLAGS: 00010246
> [  118.521991] RAX: 0000000000000000 RBX: ffff880038e87790 RCX: 0000000000000071
> [  118.521996] RDX: ffff88003f30f5d4 RSI: ffffffff82179f75 RDI: ffffffff8209b167
> [  118.522000] RBP: ffff880038e876e0 R08: ffff880038e87730 R09: ffff8800218b44c0
> [  118.522005] R10: 0000000000000000 R11: 000000000000000f R12: ffff880038ebf000
> [  118.522010] R13: ffff880038e87870 R14: ffff880038e87850 R15: 00000000ffffff9c
> [  118.522015] FS:  0000000000000000(0000) GS:ffff88003f300000(0000) knlGS:0000000000000000
> [  118.522020] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> [  118.522025] CR2: 0000000000000020 CR3: 0000000039d4f000 CR4: 00000000001007e0
> [  118.522030] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  118.522035] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [  118.522039] Stack:
> [  118.522043]  ffff880038dbc300 ffff880038e876b8 ffffffff822cf046 ffff88002e434f00
> [  118.522056]  ffff880038e87790 ffff880038e87870 ffff880038e87850 00000000ffffff9c
> [  118.522069]  ffff880038e87780 ffffffff8217d419 ffff88003b1de480 ffff88003bbf7700
> [  118.522082] Call Trace:
> [  118.522092]  [<ffffffff822cf046>] ? security_file_alloc+0x16/0x20
> [  118.522100]  [<ffffffff8217d419>] path_openat+0x69/0x4b0
> [  118.522109]  [<ffffffff826b5c7f>] ? is_connected_output_ep+0x15f/0x260
> [  118.522117]  [<ffffffff8217dae9>] do_filp_open+0x39/0x90
> [  118.522125]  [<ffffffff823316ca>] ? string.isra.3+0x3a/0xe0
> [  118.522133]  [<ffffffff82332809>] ? vsnprintf+0x209/0x620
> [  118.522141]  [<ffffffff8216ced2>] file_open_name+0xb2/0xf0
> [  118.522148]  [<ffffffff8216cf46>] filp_open+0x36/0x40
> [  118.522157]  [<ffffffff82404441>] _request_firmware+0x351/0x9c0
> [  118.522164]  [<ffffffff82404b26>] request_firmware+0x16/0x20
> [  118.522172]  [<ffffffff826dd733>] sst_request_fw+0x63/0x510
> [  118.522180]  [<ffffffff8283be10>] ? __mutex_lock_slowpath+0x280/0x3b0
> [  118.522188]  [<ffffffff826df047>] sst_load_fw+0x1f7/0x4a0
> [  118.522195]  [<ffffffff826da81f>] sst_download_fw+0xf/0x60
> [  118.522202]  [<ffffffff826db022>] intel_sst_check_device+0x92/0x1f0
> [  118.522209]  [<ffffffff826db4bd>] sst_set_generic_params+0x18d/0x500
> [  118.522218]  [<ffffffff826cee25>] sst_fill_and_send_cmd.constprop.7+0x95/0x130
> [  118.522226]  [<ffffffff826cf1dd>] sst_send_gain_cmd+0x9d/0xe0
> [  118.522233]  [<ffffffff826cf283>] sst_set_pipe_gain+0x63/0xa0
> [  118.522241]  [<ffffffff826d0665>] sst_send_pipe_gains+0xd5/0x2b0
> [  118.522248]  [<ffffffff826cb5d6>] sst_media_digital_mute+0x46/0x80
> [  118.522257]  [<ffffffff826ab5b1>] snd_soc_dai_digital_mute+0x21/0x60
> [  118.522265]  [<ffffffff826bc39d>] soc_pcm_close+0xbd/0x250
> [  118.522272]  [<ffffffff826bdf2a>] dpcm_fe_dai_close+0x7a/0x150
> [  118.522281]  [<ffffffff8267f138>] snd_pcm_release_substream+0x58/0xb0
> [  118.522289]  [<ffffffff8267f1cf>] snd_pcm_release+0x3f/0xa0//////////
> [  118.522296]  [<ffffffff8216f600>] __fput+0xf0/0x240
> [  118.522304]  [<ffffffff8216f75e>] ____fput+0xe/0x10
> [  118.522312]  [<ffffffff82090195>] task_work_run+0xa5/0xd0
> [  118.522320]  [<ffffffff82072a08>] do_exit+0x2b8/0xad0
> [  118.522327]  [<ffffffff8283d3bf>] ? __schedule+0x3df/0x820
> [  118.522335]  [<ffffffff820733af>] do_group_exit+0x3f/0xa0
> [  118.522343]  [<ffffffff82082b6e>] get_signal_to_deliver+0x24e/0x650
> [  118.522352]  [<ffffffff8200232d>] do_signal+0x4d/0x960
> [  118.522360]  [<ffffffff82097584>] ? hrtimer_start_range_ns+0x14/0x20
> [  118.522368]  [<ffffffff8262a98d>] ? binder_ioctl+0x15d/0x990
> [  118.522377]  [<ffffffff82002ca5>] do_notify_resume+0x65/0x80
> [  118.522384]  [<ffffffff82844efa>] int_signal+0x12/0x17
> 
> Signed-off-by: Leon Ma <xindong.ma@intel.com>
> Signed-off-by: Zhang Di <di.zhang@intel.com>
> Signed-off-by: Sun Zhonghua <zhonghua.sun@intel.com>
> ---
>  kernel/exit.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 1ea4369..64ba13b 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -729,6 +729,8 @@ void do_exit(long code)
>  	tsk->exit_code = code;
>  	taskstats_exit(tsk, group_dead);
>  
> +	exit_files(tsk);
> +	exit_task_work(tsk);
>  	exit_mm(tsk);
>  
>  	if (group_dead)
> @@ -737,12 +739,10 @@ void do_exit(long code)
>  
>  	exit_sem(tsk);
>  	exit_shm(tsk);
> -	exit_files(tsk);
>  	exit_fs(tsk);
>  	if (group_dead)
>  		disassociate_ctty(1);
>  	exit_task_namespaces(tsk);
> -	exit_task_work(tsk);
>  	exit_thread();
>  
>  	/*
> -- 
> 1.7.9.5
> 

RE: [PATCH] move exit_task_work() before exit_fs().

[Ma, Xindong]

> 
> On 12/26, Leon Ma wrote:
> >
> > We encountered following panic. The scenario is the process is exiting
> > and executing its task work. When closing dev node, the driver
> > triggers a firmware reload according to device status. Because task->fs is
> set to NULL in exit_fs(), panic happens.
> 
> I think this should be fixed somewhere else...
Yes, for this panic, I also think driver is not perfect and need a fix. But kernel should not add the limitation like this...
> 
> > Task work is a common interface, we should not limite the resource the
> user will utilize.
> 
> Exactly. And note that with this patch exit_mm()..disassociate_ctty() paths
> can't use task works.
I don't get this. Currently disassociate_ctty() is also called after exit_mm() and exit_task_work(). My patch didn't change this.
> 
> Not to mention that this patch moves exit_files() up, even before exit_mm(),
> without any explanation.
Moving exit_files() up is because exit_files() closes files and add tasks to task works.
> 
> Add Al. May be we can move exit_fs() down after exit_task_work(), I dunno,
> but to me it would be better to change the driver.
> 
I'm OK with this suggestion to fix this issue. I'm not sure whether in the future task work users will access other resources and expose other issues.

Re: [PATCH] move exit_task_work() before exit_fs().

[Peter Hurley]

On 12/28/2014 07:58 PM, Ma, Xindong wrote:
>>
>> On 12/26, Leon Ma wrote:
>>>
>>> We encountered following panic. The scenario is the process is exiting
>>> and executing its task work. When closing dev node, the driver
>>> triggers a firmware reload according to device status. Because task->fs is
>> set to NULL in exit_fs(), panic happens.
>>
>> I think this should be fixed somewhere else...
> Yes, for this panic, I also think driver is not perfect and need a fix. But kernel should not add the limitation like this...
>>
>>> Task work is a common interface, we should not limite the resource the
>> user will utilize.
>>
>> Exactly. And note that with this patch exit_mm()..disassociate_ctty() paths
>> can't use task works.
> I don't get this. Currently disassociate_ctty() is also called after exit_mm() and exit_task_work(). My patch didn't change this.

??

742-	if (group_dead)
743:		disassociate_ctty(1);
744-	exit_task_namespaces(tsk);
745-	exit_task_work(tsk);
746-	exit_thread();

>> Not to mention that this patch moves exit_files() up, even before exit_mm(),
>> without any explanation.
> Moving exit_files() up is because exit_files() closes files and add tasks to task works.
>>
>> Add Al. May be we can move exit_fs() down after exit_task_work(), I dunno,
>> but to me it would be better to change the driver.
>>
> I'm OK with this suggestion to fix this issue. I'm not sure whether in the future task work users will access other resources and expose other issues.
> 

RE: [PATCH] move exit_task_work() before exit_fs().

[Ma, Xindong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1917 bytes --]

> On 12/28/2014 07:58 PM, Ma, Xindong wrote:
> >>
> >> On 12/26, Leon Ma wrote:
> >>>
> >>> We encountered following panic. The scenario is the process is
> >>> exiting and executing its task work. When closing dev node, the
> >>> driver triggers a firmware reload according to device status.
> >>> Because task->fs is
> >> set to NULL in exit_fs(), panic happens.
> >>
> >> I think this should be fixed somewhere else...
> > Yes, for this panic, I also think driver is not perfect and need a fix. But
> kernel should not add the limitation like this...
> >>
> >>> Task work is a common interface, we should not limite the resource
> >>> the
> >> user will utilize.
> >>
> >> Exactly. And note that with this patch exit_mm()..disassociate_ctty()
> >> paths can't use task works.
> > I don't get this. Currently disassociate_ctty() is also called after exit_mm()
> and exit_task_work(). My patch didn't change this.
> 
> ??
> 
> 742-	if (group_dead)
> 743:		disassociate_ctty(1);
> 744-	exit_task_namespaces(tsk);
> 745-	exit_task_work(tsk);
> 746-	exit_thread();
Sorry, I was checking the old source code when applying this mail. I did a check on latest code, and did not find disassociate_ctty() was using task work. So why this matters?
> 
> >> Not to mention that this patch moves exit_files() up, even before
> >> exit_mm(), without any explanation.
> > Moving exit_files() up is because exit_files() closes files and add tasks to
> task works.
> >>
> >> Add Al. May be we can move exit_fs() down after exit_task_work(), I
> >> dunno, but to me it would be better to change the driver.
> >>
> > I'm OK with this suggestion to fix this issue. I'm not sure whether in the
> future task work users will access other resources and expose other issues.
> >

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] move exit_task_work() before exit_fs().

[Al Viro]

On Fri, Dec 26, 2014 at 03:45:25PM +0800, Leon Ma wrote:
> We encountered following panic. The scenario is the process is exiting and executing its
> task work. When closing dev node, the driver triggers a firmware reload according to device
> status. Because task->fs is set to NULL in exit_fs(), panic happens.
> Task work is a common interface, we should not limite the resource the user will utilize.

Fix your driver.  Forget ->fs being NULL; what will happen if your process
is chrooted?

RE: [PATCH] move exit_task_work() before exit_fs().

[Ma, Xindong]

> On Fri, Dec 26, 2014 at 03:45:25PM +0800, Leon Ma wrote:
> > We encountered following panic. The scenario is the process is exiting
> > and executing its task work. When closing dev node, the driver
> > triggers a firmware reload according to device status. Because task->fs is
> set to NULL in exit_fs(), panic happens.
> > Task work is a common interface, we should not limite the resource the
> user will utilize.
> 
> Fix your driver.  Forget ->fs being NULL; what will happen if your process is
> chrooted?
Thanks, But I'm not clear what is the limitation added to chroot env?

Re: [PATCH] move exit_task_work() before exit_fs().

[Al Viro]

On Mon, Dec 29, 2014 at 01:33:37AM +0000, Ma, Xindong wrote:
> > On Fri, Dec 26, 2014 at 03:45:25PM +0800, Leon Ma wrote:
> > > We encountered following panic. The scenario is the process is exiting
> > > and executing its task work. When closing dev node, the driver
> > > triggers a firmware reload according to device status. Because task->fs is
> > set to NULL in exit_fs(), panic happens.
> > > Task work is a common interface, we should not limite the resource the
> > user will utilize.
> > 
> > Fix your driver.  Forget ->fs being NULL; what will happen if your process is
> > chrooted?
> Thanks, But I'm not clear what is the limitation added to chroot env?

???

How about "the pathname of firmware apparently will be looked up inside the
chroot"?  Look, this is completely broken - you *can't* assume anything about
the fs context in which ->release() will be run, period.  It's not guaranteed
to have anything in common with the environment in which the file had been
opened, BTW - there might literally be not a single filesystem in common for
both.

This "reload firmware in ->release()" is absolutely braindead.  The fact that
it as much as dereferences current->fs demonstrates that it's broken.  Don't
do that.