linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
@ 2012-01-02  9:38 민찬호
  2012-01-02  9:57 ` Wu Fengguang
  0 siblings, 1 reply; 10+ messages in thread
From: 민찬호 @ 2012-01-02  9:38 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: 'Jens Axboe', 'Wu Fengguang',
	'Andrew Morton'

from Chanho Min <chanho.min@lge.com>

System may crash in backing-dev.c when removal SCSI device is detached.
bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
Shortly afterward, If 'wb->wakeup_timer' is expired before
del_timer()/bdi_forker_thread,
wakeup_timer_fn() may wake up the dead thread which cause the crash.
'bdi->wb.task' should be NULL as this patch.

Signed-off-by: Chanho Min <chanho.min@lge.com>
---
 mm/backing-dev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 71034f4..4378a5e 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
*bdi)
        if (bdi->wb.task) {
                thaw_process(bdi->wb.task);
                kthread_stop(bdi->wb.task);
+               bdi->wb.task = NULL;
        }
 }

-- 
1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
@ 2012-01-02  9:38 민찬호
  0 siblings, 0 replies; 10+ messages in thread
From: 민찬호 @ 2012-01-02  9:38 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: 'Jens Axboe', 'Wu Fengguang',
	'Andrew Morton'

from Chanho Min <chanho.min@lge.com>

System may crash in backing-dev.c when removal SCSI device is detached.
bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
Shortly afterward, If 'wb->wakeup_timer' is expired before
del_timer()/bdi_forker_thread,
wakeup_timer_fn() may wake up the dead thread which cause the crash.
'bdi->wb.task' should be NULL as this patch.

Signed-off-by: Chanho Min <chanho.min@lge.com>
---
 mm/backing-dev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 71034f4..4378a5e 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
*bdi)
        if (bdi->wb.task) {
                thaw_process(bdi->wb.task);
                kthread_stop(bdi->wb.task);
+               bdi->wb.task = NULL;
        }
 }

-- 
1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
  2012-01-02  9:38 [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached 민찬호
@ 2012-01-02  9:57 ` Wu Fengguang
  2012-01-03  3:23   ` Chanho Min
  2012-01-03  3:23   ` Chanho Min
  0 siblings, 2 replies; 10+ messages in thread
From: Wu Fengguang @ 2012-01-02  9:57 UTC (permalink / raw)
  To: ����ȣ
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton'

On Mon, Jan 02, 2012 at 06:38:21PM +0900, i? 1/2 i? 1/2 i? 1/2 i? 1/2 EGBP wrote:
> from Chanho Min <chanho.min@lge.com>
> 
> System may crash in backing-dev.c when removal SCSI device is detached.
> bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
> Shortly afterward, If 'wb->wakeup_timer' is expired before
> del_timer()/bdi_forker_thread,
> wakeup_timer_fn() may wake up the dead thread which cause the crash.
> 'bdi->wb.task' should be NULL as this patch.

Is it some race condition between del_timer() and del_timer_sync()?

bdi_unregister() calls 

        del_timer_sync
        bdi_wb_shutdown
            kthread_stop

in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is
no longer called to access the stopped task.

Thanks,
Fengguang


> Signed-off-by: Chanho Min <chanho.min@lge.com>
> ---
>  mm/backing-dev.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 71034f4..4378a5e 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
> *bdi)
>         if (bdi->wb.task) {
>                 thaw_process(bdi->wb.task);
>                 kthread_stop(bdi->wb.task);
> +               bdi->wb.task = NULL;
>         }
>  }
> 
> -- 
> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
  2012-01-02  9:57 ` Wu Fengguang
@ 2012-01-03  3:23   ` Chanho Min
  2012-01-03  4:49     ` Wu Fengguang
  2012-01-03  3:23   ` Chanho Min
  1 sibling, 1 reply; 10+ messages in thread
From: Chanho Min @ 2012-01-03  3:23 UTC (permalink / raw)
  To: 'Wu Fengguang'
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton'

>On Mon, Jan 02, 2012 at 06:38:21PM +0900, ����ȣ wrote:
>> from Chanho Min <chanho.min@lge.com>
>>
>> System may crash in backing-dev.c when removal SCSI device is detached.
>> bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
>> Shortly afterward, If 'wb->wakeup_timer' is expired before
>> del_timer()/bdi_forker_thread,
>> wakeup_timer_fn() may wake up the dead thread which cause the crash.
>> 'bdi->wb.task' should be NULL as this patch.
>
>Is it some race condition between del_timer() and del_timer_sync()?
>
>bdi_unregister() calls
>
>        del_timer_sync
>        bdi_wb_shutdown
>            kthread_stop
>
>in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is
>no longer called to access the stopped task.
>

It is not race condition. This happens when USB is removed during write-access.
bdi_wakeup_thread_delayed is called after kthread_stop, and timer is activated again.

	bdi_unregister
		kthread_stop
	bdi_wakeup_thread_delayed (sys_write mostly calls this)
	timer fires

Anyway,Is this safeguard to prevent from waking up killed thread?

Thanks,
Chanho

>Thanks,
>Fengguang
>
>
>> Signed-off-by: Chanho Min <chanho.min@lge.com>
>> ---
>>  mm/backing-dev.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> index 71034f4..4378a5e 100644
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
>> *bdi)
>>         if (bdi->wb.task) {
>>                 thaw_process(bdi->wb.task);
>>                 kthread_stop(bdi->wb.task);
>> +               bdi->wb.task = NULL;
>>         }
>>  }
>>
>> --
>> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
  2012-01-02  9:57 ` Wu Fengguang
  2012-01-03  3:23   ` Chanho Min
@ 2012-01-03  3:23   ` Chanho Min
  1 sibling, 0 replies; 10+ messages in thread
From: Chanho Min @ 2012-01-03  3:23 UTC (permalink / raw)
  To: 'Wu Fengguang'
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton'

>On Mon, Jan 02, 2012 at 06:38:21PM +0900, ����ȣ wrote:
>> from Chanho Min <chanho.min@lge.com>
>>
>> System may crash in backing-dev.c when removal SCSI device is detached.
>> bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
>> Shortly afterward, If 'wb->wakeup_timer' is expired before
>> del_timer()/bdi_forker_thread,
>> wakeup_timer_fn() may wake up the dead thread which cause the crash.
>> 'bdi->wb.task' should be NULL as this patch.
>
>Is it some race condition between del_timer() and del_timer_sync()?
>
>bdi_unregister() calls
>
>        del_timer_sync
>        bdi_wb_shutdown
>            kthread_stop
>
>in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is
>no longer called to access the stopped task.
>

It is not race condition. This happens when USB is removed during write-access.
bdi_wakeup_thread_delayed is called after kthread_stop, and timer is activated again.

	bdi_unregister
		kthread_stop
	bdi_wakeup_thread_delayed (sys_write mostly calls this)
	timer fires

Anyway,Is this safeguard to prevent from waking up killed thread?

Thanks,
Chanho

>Thanks,
>Fengguang
>
>
>> Signed-off-by: Chanho Min <chanho.min@lge.com>
>> ---
>>  mm/backing-dev.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> index 71034f4..4378a5e 100644
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
>> *bdi)
>>         if (bdi->wb.task) {
>>                 thaw_process(bdi->wb.task);
>>                 kthread_stop(bdi->wb.task);
>> +               bdi->wb.task = NULL;
>>         }
>>  }
>>
>> --
>> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
@ 2012-01-03  3:47 'Chanho Min'
  0 siblings, 0 replies; 10+ messages in thread
From: 'Chanho Min' @ 2012-01-03  3:47 UTC (permalink / raw)
  To: 'Wu Fengguang'
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton'

>On Mon, Jan 02, 2012 at 06:38:21PM +0900,     ȣ wrote:
>> from Chanho Min <chanho.min@lge.com>
>>
>> System may crash in backing-dev.c when removal SCSI device is detached.
>> bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
>> Shortly afterward, If 'wb->wakeup_timer' is expired before 
>> del_timer()/bdi_forker_thread,
>> wakeup_timer_fn() may wake up the dead thread which cause the crash.
>> 'bdi->wb.task' should be NULL as this patch.
>
>Is it some race condition between del_timer() and del_timer_sync()?
>
>bdi_unregister() calls
>
>        del_timer_sync
>        bdi_wb_shutdown
>            kthread_stop
>
>in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is no 
>longer called to access the stopped task.
>

It is not race condition. This happens when USB is removed during write-access.
bdi_wakeup_thread_delayed is called after kthread_stop, and timer is activated again.

	bdi_unregister
		kthread_stop
	bdi_wakeup_thread_delayed (sys_write mostly calls this)
	timer fires

Anyway,Is this safeguard to prevent from waking up killed thread?

Thanks,
Chanho

>Thanks,
>Fengguang
>
>
>> Signed-off-by: Chanho Min <chanho.min@lge.com>
>> ---
>>  mm/backing-dev.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 
>> 71034f4..4378a5e 100644
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct 
>> backing_dev_info
>> *bdi)
>>         if (bdi->wb.task) {
>>                 thaw_process(bdi->wb.task);
>>                 kthread_stop(bdi->wb.task);
>> +               bdi->wb.task = NULL;
>>         }
>>  }
>>
>> --
>> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
@ 2012-01-03  3:47 'Chanho Min'
  0 siblings, 0 replies; 10+ messages in thread
From: 'Chanho Min' @ 2012-01-03  3:47 UTC (permalink / raw)
  To: 'Wu Fengguang'
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton'

>On Mon, Jan 02, 2012 at 06:38:21PM +0900,     ȣ wrote:
>> from Chanho Min <chanho.min@lge.com>
>>
>> System may crash in backing-dev.c when removal SCSI device is detached.
>> bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
>> Shortly afterward, If 'wb->wakeup_timer' is expired before 
>> del_timer()/bdi_forker_thread,
>> wakeup_timer_fn() may wake up the dead thread which cause the crash.
>> 'bdi->wb.task' should be NULL as this patch.
>
>Is it some race condition between del_timer() and del_timer_sync()?
>
>bdi_unregister() calls
>
>        del_timer_sync
>        bdi_wb_shutdown
>            kthread_stop
>
>in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is no 
>longer called to access the stopped task.
>

It is not race condition. This happens when USB is removed during write-access.
bdi_wakeup_thread_delayed is called after kthread_stop, and timer is activated again.

	bdi_unregister
		kthread_stop
	bdi_wakeup_thread_delayed (sys_write mostly calls this)
	timer fires

Anyway,Is this safeguard to prevent from waking up killed thread?

Thanks,
Chanho

>Thanks,
>Fengguang
>
>
>> Signed-off-by: Chanho Min <chanho.min@lge.com>
>> ---
>>  mm/backing-dev.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 
>> 71034f4..4378a5e 100644
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct 
>> backing_dev_info
>> *bdi)
>>         if (bdi->wb.task) {
>>                 thaw_process(bdi->wb.task);
>>                 kthread_stop(bdi->wb.task);
>> +               bdi->wb.task = NULL;
>>         }
>>  }
>>
>> --
>> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
  2012-01-03  3:23   ` Chanho Min
@ 2012-01-03  4:49     ` Wu Fengguang
  2012-01-03 11:22       ` Chanho Min
  2012-01-03 11:22       ` Chanho Min
  0 siblings, 2 replies; 10+ messages in thread
From: Wu Fengguang @ 2012-01-03  4:49 UTC (permalink / raw)
  To: Chanho Min
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton', Rabin Vincent, Linus Walleij

On Tue, Jan 03, 2012 at 12:23:44PM +0900, Chanho Min wrote:
> >On Mon, Jan 02, 2012 at 06:38:21PM +0900, i? 1/2 i? 1/2 i? 1/2 i? 1/2 EGBP wrote:
> >> from Chanho Min <chanho.min@lge.com>
> >>
> >> System may crash in backing-dev.c when removal SCSI device is detached.
> >> bdi task is killed by bdi_unregister()/'khubd', but task's point remains.
> >> Shortly afterward, If 'wb->wakeup_timer' is expired before
> >> del_timer()/bdi_forker_thread,
> >> wakeup_timer_fn() may wake up the dead thread which cause the crash.
> >> 'bdi->wb.task' should be NULL as this patch.
> >
> >Is it some race condition between del_timer() and del_timer_sync()?
> >
> >bdi_unregister() calls
> >
> >        del_timer_sync
> >        bdi_wb_shutdown
> >            kthread_stop
> >
> >in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is
> >no longer called to access the stopped task.
> >
> 
> It is not race condition. This happens when USB is removed during write-access.
> bdi_wakeup_thread_delayed is called after kthread_stop, and timer is activated again.
> 
> 	bdi_unregister
> 		kthread_stop
> 	bdi_wakeup_thread_delayed (sys_write mostly calls this)
> 	timer fires

Ah OK, the timer could be restarted in the mean while, which breaks
the synchronization rule in del_timer_sync().

I noticed a related fix is merged recently, does your test kernel
contain this commit?

commit 7a401a972df8e184b3d1a3fc958c0a4ddee8d312
Author: Rabin Vincent <rabin.vincent@stericsson.com>
Date:   Fri Nov 11 13:29:04 2011 +0100

    backing-dev: ensure wakeup_timer is deleted

> Anyway,Is this safeguard to prevent from waking up killed thread?

This patch makes no guarantee wakeup_timer_fn() will see NULL
bdi->wb.task before the task is stopped, so there is still race
conditions. And still, the complete fix would be to prevent
wakeup_timer_fn() from being called at all.

Thanks,
Fengguang

> >> Signed-off-by: Chanho Min <chanho.min@lge.com>
> >> ---
> >>  mm/backing-dev.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> >> index 71034f4..4378a5e 100644
> >> --- a/mm/backing-dev.c
> >> +++ b/mm/backing-dev.c
> >> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
> >> *bdi)
> >>         if (bdi->wb.task) {
> >>                 thaw_process(bdi->wb.task);
> >>                 kthread_stop(bdi->wb.task);
> >> +               bdi->wb.task = NULL;
> >>         }
> >>  }
> >>
> >> --
> >> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
  2012-01-03  4:49     ` Wu Fengguang
  2012-01-03 11:22       ` Chanho Min
@ 2012-01-03 11:22       ` Chanho Min
  1 sibling, 0 replies; 10+ messages in thread
From: Chanho Min @ 2012-01-03 11:22 UTC (permalink / raw)
  To: 'Wu Fengguang'
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton', 'Rabin Vincent',
	'Linus Walleij'

>On Tue, Jan 03, 2012 at 12:23:44PM +0900, Chanho Min wrote:
>> >On Mon, Jan 02, 2012 at 06:38:21PM +0900, ����ȣ wrote:
>> >> from Chanho Min <chanho.min@lge.com>
>> >>
>> >> System may crash in backing-dev.c when removal SCSI device is detached.
>> >> bdi task is killed by bdi_unregister()/'khubd', but task's point
>remains.
>> >> Shortly afterward, If 'wb->wakeup_timer' is expired before
>> >> del_timer()/bdi_forker_thread,
>> >> wakeup_timer_fn() may wake up the dead thread which cause the crash.
>> >> 'bdi->wb.task' should be NULL as this patch.
>> >
>> >Is it some race condition between del_timer() and del_timer_sync()?
>> >
>> >bdi_unregister() calls
>> >
>> >        del_timer_sync
>> >        bdi_wb_shutdown
>> >            kthread_stop
>> >
>> >in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is
>> >no longer called to access the stopped task.
>> >
>>
>> It is not race condition. This happens when USB is removed during write-
>access.
>> bdi_wakeup_thread_delayed is called after kthread_stop, and timer is
>activated again.
>>
>> 	bdi_unregister
>> 		kthread_stop
>> 	bdi_wakeup_thread_delayed (sys_write mostly calls this)
>> 	timer fires
>
>Ah OK, the timer could be restarted in the mean while, which breaks
>the synchronization rule in del_timer_sync().
>
>I noticed a related fix is merged recently, does your test kernel
>contain this commit?
>

No, I will try to reproduce with this patch. 
But, bdi_destroy is not called during write-access. Same result is expected.

>commit 7a401a972df8e184b3d1a3fc958c0a4ddee8d312
>Author: Rabin Vincent <rabin.vincent@stericsson.com>
>Date:   Fri Nov 11 13:29:04 2011 +0100
>
>    backing-dev: ensure wakeup_timer is deleted
>
>> Anyway,Is this safeguard to prevent from waking up killed thread?
>
>This patch makes no guarantee wakeup_timer_fn() will see NULL
>bdi->wb.task before the task is stopped, so there is still race
>conditions. And still, the complete fix would be to prevent
>wakeup_timer_fn() from being called at all.

If wakeup_timer_fn() see NULL bdi->wb.task, wakeup_timer_fn regards task as killed
and wake up forker thread instead of the defined thread.
Is this intended behavior of the bdi?

>
>Thanks,
>Fengguang
>
>> >> Signed-off-by: Chanho Min <chanho.min@lge.com>
>> >> ---
>> >>  mm/backing-dev.c |    1 +
>> >>  1 files changed, 1 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> >> index 71034f4..4378a5e 100644
>> >> --- a/mm/backing-dev.c
>> >> +++ b/mm/backing-dev.c
>> >> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
>> >> *bdi)
>> >>         if (bdi->wb.task) {
>> >>                 thaw_process(bdi->wb.task);
>> >>                 kthread_stop(bdi->wb.task);
>> >> +               bdi->wb.task = NULL;
>> >>         }
>> >>  }
>> >>
>> >> --
>> >> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached
  2012-01-03  4:49     ` Wu Fengguang
@ 2012-01-03 11:22       ` Chanho Min
  2012-01-03 11:22       ` Chanho Min
  1 sibling, 0 replies; 10+ messages in thread
From: Chanho Min @ 2012-01-03 11:22 UTC (permalink / raw)
  To: 'Wu Fengguang'
  Cc: linux-mm, linux-kernel, 'Jens Axboe',
	'Andrew Morton', 'Rabin Vincent',
	'Linus Walleij'

>On Tue, Jan 03, 2012 at 12:23:44PM +0900, Chanho Min wrote:
>> >On Mon, Jan 02, 2012 at 06:38:21PM +0900, ����ȣ wrote:
>> >> from Chanho Min <chanho.min@lge.com>
>> >>
>> >> System may crash in backing-dev.c when removal SCSI device is detached.
>> >> bdi task is killed by bdi_unregister()/'khubd', but task's point
>remains.
>> >> Shortly afterward, If 'wb->wakeup_timer' is expired before
>> >> del_timer()/bdi_forker_thread,
>> >> wakeup_timer_fn() may wake up the dead thread which cause the crash.
>> >> 'bdi->wb.task' should be NULL as this patch.
>> >
>> >Is it some race condition between del_timer() and del_timer_sync()?
>> >
>> >bdi_unregister() calls
>> >
>> >        del_timer_sync
>> >        bdi_wb_shutdown
>> >            kthread_stop
>> >
>> >in turn, and del_timer_sync() should guarantee wakeup_timer_fn() is
>> >no longer called to access the stopped task.
>> >
>>
>> It is not race condition. This happens when USB is removed during write-
>access.
>> bdi_wakeup_thread_delayed is called after kthread_stop, and timer is
>activated again.
>>
>> 	bdi_unregister
>> 		kthread_stop
>> 	bdi_wakeup_thread_delayed (sys_write mostly calls this)
>> 	timer fires
>
>Ah OK, the timer could be restarted in the mean while, which breaks
>the synchronization rule in del_timer_sync().
>
>I noticed a related fix is merged recently, does your test kernel
>contain this commit?
>

No, I will try to reproduce with this patch. 
But, bdi_destroy is not called during write-access. Same result is expected.

>commit 7a401a972df8e184b3d1a3fc958c0a4ddee8d312
>Author: Rabin Vincent <rabin.vincent@stericsson.com>
>Date:   Fri Nov 11 13:29:04 2011 +0100
>
>    backing-dev: ensure wakeup_timer is deleted
>
>> Anyway,Is this safeguard to prevent from waking up killed thread?
>
>This patch makes no guarantee wakeup_timer_fn() will see NULL
>bdi->wb.task before the task is stopped, so there is still race
>conditions. And still, the complete fix would be to prevent
>wakeup_timer_fn() from being called at all.

If wakeup_timer_fn() see NULL bdi->wb.task, wakeup_timer_fn regards task as killed
and wake up forker thread instead of the defined thread.
Is this intended behavior of the bdi?

>
>Thanks,
>Fengguang
>
>> >> Signed-off-by: Chanho Min <chanho.min@lge.com>
>> >> ---
>> >>  mm/backing-dev.c |    1 +
>> >>  1 files changed, 1 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> >> index 71034f4..4378a5e 100644
>> >> --- a/mm/backing-dev.c
>> >> +++ b/mm/backing-dev.c
>> >> @@ -607,6 +607,7 @@ static void bdi_wb_shutdown(struct backing_dev_info
>> >> *bdi)
>> >>         if (bdi->wb.task) {
>> >>                 thaw_process(bdi->wb.task);
>> >>                 kthread_stop(bdi->wb.task);
>> >> +               bdi->wb.task = NULL;
>> >>         }
>> >>  }
>> >>
>> >> --
>> >> 1.7.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-01-03 11:23 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-02  9:38 [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached 민찬호
2012-01-02  9:57 ` Wu Fengguang
2012-01-03  3:23   ` Chanho Min
2012-01-03  4:49     ` Wu Fengguang
2012-01-03 11:22       ` Chanho Min
2012-01-03 11:22       ` Chanho Min
2012-01-03  3:23   ` Chanho Min
  -- strict thread matches above, loose matches on Subject: below --
2012-01-02  9:38 민찬호
2012-01-03  3:47 'Chanho Min'
2012-01-03  3:47 'Chanho Min'

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).