[PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[민찬호]

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>

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

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.

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

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

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>

RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Chanho Min]

>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>

RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Chanho Min]

>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>

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

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?

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

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

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>

RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Chanho Min]

>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>

RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Chanho Min]

>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>

[PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[민찬호]

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>

RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

['Chanho Min']

>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>

RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

['Chanho Min']

>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>

RE: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Chanho Min]

>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

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Rabin Vincent]

On Thu, Jan 5, 2012 at 14:19, Chanho Min <chanho0207@gmail.com> wrote:
>>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.
>>
>>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.

I agree. 7a401a972df8e184b3d1a3fc958c0a4ddee8d312 only addressed the
problem of the bdi being destroyed with an active timer, but there are
other races that could happen before that.

>>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?

This appears to be the intended behaviour before, but certainly not
after the bdi is unregistered, since anyway the forker thread will not
find the bdi on the list.  In fact, if tracing is enabled the kernel
crashes because dev_name() is called on a NULL bdi->dev from the
wake_forker_thread tracepoint.

The following patch should address these issues:

8<---------------------------
>From 271f92d34b661d701eaad9b262423de5dba1cc11 Mon Sep 17 00:00:00 2001
From: Rabin Vincent <rabin@rab.in>
Date: Sun, 15 Jan 2012 15:30:40 +0530
Subject: [PATCH] backing-dev: fix wakeup timer races with bdi_unregister()

While 7a401a972df8e18 ("backing-dev: ensure wakeup_timer is deleted")
addressed the problem of the bdi being freed with a queued wakeup
timer, there are other races that could happen if the wakeup timer
expires after/during bdi_unregister(), before bdi_destroy() is called.

wakeup_timer_fn() could attempt to wakeup a task which has already has
been freed, or could access a NULL bdi->dev via the wake_forker_thread
tracepoint.

Reported-by: Chanho Min <chanho.min@lge.com>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 mm/backing-dev.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 71034f4..a39ad70 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -318,7 +318,7 @@ static void wakeup_timer_fn(unsigned long data)
 	if (bdi->wb.task) {
 		trace_writeback_wake_thread(bdi);
 		wake_up_process(bdi->wb.task);
-	} else {
+	} else if (bdi->dev) {
 		/*
 		 * When bdi tasks are inactive for long time, they are killed.
 		 * In this case we have to wake-up the forker thread which
@@ -584,6 +584,8 @@ EXPORT_SYMBOL(bdi_register_dev);
  */
 static void bdi_wb_shutdown(struct backing_dev_info *bdi)
 {
+	struct task_struct *task = NULL;
+
 	if (!bdi_cap_writeback_dirty(bdi))
 		return;

@@ -604,9 +606,14 @@ static void bdi_wb_shutdown(struct backing_dev_info *bdi)
 	 * unfreeze of the thread before calling kthread_stop(), otherwise
 	 * it would never exet if it is currently stuck in the refrigerator.
 	 */
-	if (bdi->wb.task) {
-		thaw_process(bdi->wb.task);
-		kthread_stop(bdi->wb.task);
+	spin_lock_bh(&bdi->wb_lock);
+	task = bdi->wb.task;
+	bdi->wb.task = NULL;
+	spin_unlock_bh(&bdi->wb_lock);
+
+	if (task) {
+		thaw_process(task);
+		kthread_stop(task);
 	}
 }

@@ -637,7 +644,9 @@ void bdi_unregister(struct backing_dev_info *bdi)
 			bdi_wb_shutdown(bdi);
 		bdi_debug_unregister(bdi);
 		device_unregister(bdi->dev);
+		spin_lock_bh(&bdi->wb_lock);
 		bdi->dev = NULL;
+		spin_unlock_bh(&bdi->wb_lock);
 	}
 }
 EXPORT_SYMBOL(bdi_unregister);
-- 
1.7.7.3

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

On Sun, Jan 15, 2012 at 03:58:43PM +0530, Rabin Vincent wrote:
> On Thu, Jan 5, 2012 at 14:19, Chanho Min <chanho0207@gmail.com> wrote:
> >>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.
> >>
> >>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.
> 
> I agree. 7a401a972df8e184b3d1a3fc958c0a4ddee8d312 only addressed the
> problem of the bdi being destroyed with an active timer, but there are
> other races that could happen before that.
> 
> >>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?
> 
> This appears to be the intended behaviour before, but certainly not
> after the bdi is unregistered, since anyway the forker thread will not
> find the bdi on the list.  In fact, if tracing is enabled the kernel
> crashes because dev_name() is called on a NULL bdi->dev from the
> wake_forker_thread tracepoint.

Right.

> The following patch should address these issues:
> 
> 8<---------------------------
> >From 271f92d34b661d701eaad9b262423de5dba1cc11 Mon Sep 17 00:00:00 2001
> From: Rabin Vincent <rabin@rab.in>
> Date: Sun, 15 Jan 2012 15:30:40 +0530
> Subject: [PATCH] backing-dev: fix wakeup timer races with bdi_unregister()
> 
> While 7a401a972df8e18 ("backing-dev: ensure wakeup_timer is deleted")
> addressed the problem of the bdi being freed with a queued wakeup
> timer, there are other races that could happen if the wakeup timer
> expires after/during bdi_unregister(), before bdi_destroy() is called.
> 
> wakeup_timer_fn() could attempt to wakeup a task which has already has
> been freed, or could access a NULL bdi->dev via the wake_forker_thread
> tracepoint.
> 
> Reported-by: Chanho Min <chanho.min@lge.com>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>

>  mm/backing-dev.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 71034f4..a39ad70 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -318,7 +318,7 @@ static void wakeup_timer_fn(unsigned long data)
>  	if (bdi->wb.task) {
>  		trace_writeback_wake_thread(bdi);
>  		wake_up_process(bdi->wb.task);
> -	} else {
> +	} else if (bdi->dev) {
>  		/*
>  		 * When bdi tasks are inactive for long time, they are killed.
>  		 * In this case we have to wake-up the forker thread which
> @@ -584,6 +584,8 @@ EXPORT_SYMBOL(bdi_register_dev);
>   */
>  static void bdi_wb_shutdown(struct backing_dev_info *bdi)
>  {
> +	struct task_struct *task = NULL;

NULL not necessary?

Thanks,
Fengguang

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Rabin Vincent]

On Sun, Jan 15, 2012 at 08:58:53PM +0800, Wu Fengguang wrote:
> On Sun, Jan 15, 2012 at 03:58:43PM +0530, Rabin Vincent wrote:
> > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > index 71034f4..a39ad70 100644
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -318,7 +318,7 @@ static void wakeup_timer_fn(unsigned long data)
> >  	if (bdi->wb.task) {
> >  		trace_writeback_wake_thread(bdi);
> >  		wake_up_process(bdi->wb.task);
> > -	} else {
> > +	} else if (bdi->dev) {
> >  		/*
> >  		 * When bdi tasks are inactive for long time, they are killed.
> >  		 * In this case we have to wake-up the forker thread which
> > @@ -584,6 +584,8 @@ EXPORT_SYMBOL(bdi_register_dev);
> >   */
> >  static void bdi_wb_shutdown(struct backing_dev_info *bdi)
> >  {
> > +	struct task_struct *task = NULL;
> 
> NULL not necessary?

Will fix.

Also, I notice another problem here.  bdi->dev should be made NULL before the
device is unregistered, not after:

		if (!bdi_cap_flush_forker(bdi))
			bdi_wb_shutdown(bdi);
		bdi_debug_unregister(bdi);
		device_unregister(bdi->dev);
		bdi->dev = NULL;

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

On Sun, Jan 15, 2012 at 09:11:07PM +0530, Rabin Vincent wrote:
> On Sun, Jan 15, 2012 at 08:58:53PM +0800, Wu Fengguang wrote:
> > On Sun, Jan 15, 2012 at 03:58:43PM +0530, Rabin Vincent wrote:
> > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > > index 71034f4..a39ad70 100644
> > > --- a/mm/backing-dev.c
> > > +++ b/mm/backing-dev.c
> > > @@ -318,7 +318,7 @@ static void wakeup_timer_fn(unsigned long data)
> > >  	if (bdi->wb.task) {
> > >  		trace_writeback_wake_thread(bdi);
> > >  		wake_up_process(bdi->wb.task);
> > > -	} else {
> > > +	} else if (bdi->dev) {
> > >  		/*
> > >  		 * When bdi tasks are inactive for long time, they are killed.
> > >  		 * In this case we have to wake-up the forker thread which
> > > @@ -584,6 +584,8 @@ EXPORT_SYMBOL(bdi_register_dev);
> > >   */
> > >  static void bdi_wb_shutdown(struct backing_dev_info *bdi)
> > >  {
> > > +	struct task_struct *task = NULL;
> > 
> > NULL not necessary?
> 
> Will fix.
> 
> Also, I notice another problem here.  bdi->dev should be made NULL before the
> device is unregistered, not after:

Yes, good catch!

> 		if (!bdi_cap_flush_forker(bdi))
> 			bdi_wb_shutdown(bdi);
> 		bdi_debug_unregister(bdi);
> 		device_unregister(bdi->dev);
> 		bdi->dev = NULL;

Thanks,
Fengguang

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Chanho Min]

On Mon, Jan 16, 2012 at 11:53 AM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> On Sun, Jan 15, 2012 at 09:11:07PM +0530, Rabin Vincent wrote:
>> On Sun, Jan 15, 2012 at 08:58:53PM +0800, Wu Fengguang wrote:
>> > On Sun, Jan 15, 2012 at 03:58:43PM +0530, Rabin Vincent wrote:
>> > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> > > index 71034f4..a39ad70 100644
>> > > --- a/mm/backing-dev.c
>> > > +++ b/mm/backing-dev.c
>> > > @@ -318,7 +318,7 @@ static void wakeup_timer_fn(unsigned long data)
>> > >   if (bdi->wb.task) {
>> > >           trace_writeback_wake_thread(bdi);
>> > >           wake_up_process(bdi->wb.task);
>> > > - } else {
>> > > + } else if (bdi->dev) {
>> > >           /*
>> > >            * When bdi tasks are inactive for long time, they are killed.
>> > >            * In this case we have to wake-up the forker thread which
>> > > @@ -584,6 +584,8 @@ EXPORT_SYMBOL(bdi_register_dev);
>> > >   */
>> > >  static void bdi_wb_shutdown(struct backing_dev_info *bdi)
>> > >  {
>> > > + struct task_struct *task = NULL;

Thanks, I fully understand.
In addition, Would not bdi_wakeup_flusher also be fixed?

>From dc7d4e86911c0a7ea35043485b04e8a09aa74ffd Mon Sep 17 00:00:00 2001
From: Chanho Min <chanho.min@lge.com>
Date: Mon, 16 Jan 2012 14:14:48 +0900
Subject: [PATCH] backing-dev: add for 'fix wakeup timer races with
bdi_unregister()'

Signed-off-by: Chanho Min <chanho.min@lge.com>
---
 fs/fs-writeback.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index f855916..ee40833 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -97,7 +97,7 @@ static void bdi_wakeup_flusher(struct backing_dev_info *bdi)
 {
        if (bdi->wb.task) {
                wake_up_process(bdi->wb.task);
-       } else {
+       } else if (bdi->dev) {
                /*
                 * The bdi thread isn't there, wake up the forker thread which
                 * will create and run it.
-- 
1.7.0.4

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

On Mon, Jan 16, 2012 at 02:28:30PM +0900, Chanho Min wrote:
> On Mon, Jan 16, 2012 at 11:53 AM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> > On Sun, Jan 15, 2012 at 09:11:07PM +0530, Rabin Vincent wrote:
> >> On Sun, Jan 15, 2012 at 08:58:53PM +0800, Wu Fengguang wrote:
> >> > On Sun, Jan 15, 2012 at 03:58:43PM +0530, Rabin Vincent wrote:
> >> > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> >> > > index 71034f4..a39ad70 100644
> >> > > --- a/mm/backing-dev.c
> >> > > +++ b/mm/backing-dev.c
> >> > > @@ -318,7 +318,7 @@ static void wakeup_timer_fn(unsigned long data)
> >> > >   if (bdi->wb.task) {
> >> > >           trace_writeback_wake_thread(bdi);
> >> > >           wake_up_process(bdi->wb.task);
> >> > > - } else {
> >> > > + } else if (bdi->dev) {
> >> > >           /*
> >> > >            * When bdi tasks are inactive for long time, they are killed.
> >> > >            * In this case we have to wake-up the forker thread which
> >> > > @@ -584,6 +584,8 @@ EXPORT_SYMBOL(bdi_register_dev);
> >> > >   */
> >> > >  static void bdi_wb_shutdown(struct backing_dev_info *bdi)
> >> > >  {
> >> > > + struct task_struct *task = NULL;
> 
> Thanks, I fully understand.
> In addition, Would not bdi_wakeup_flusher also be fixed?

Yes, indeed!

Thanks,
Fengguang

> >From dc7d4e86911c0a7ea35043485b04e8a09aa74ffd Mon Sep 17 00:00:00 2001
> From: Chanho Min <chanho.min@lge.com>
> Date: Mon, 16 Jan 2012 14:14:48 +0900
> Subject: [PATCH] backing-dev: add for 'fix wakeup timer races with
> bdi_unregister()'
> 
> Signed-off-by: Chanho Min <chanho.min@lge.com>
> ---
>  fs/fs-writeback.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index f855916..ee40833 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -97,7 +97,7 @@ static void bdi_wakeup_flusher(struct backing_dev_info *bdi)
>  {
>         if (bdi->wb.task) {
>                 wake_up_process(bdi->wb.task);
> -       } else {
> +       } else if (bdi->dev) {
>                 /*
>                  * The bdi thread isn't there, wake up the forker thread which
>                  * will create and run it.
> -- 
> 1.7.0.4

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Wu Fengguang]

On Mon, Jan 16, 2012 at 01:50:44PM +0800, Wu Fengguang wrote:
> On Mon, Jan 16, 2012 at 02:28:30PM +0900, Chanho Min wrote:
> > On Mon, Jan 16, 2012 at 11:53 AM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> > > On Sun, Jan 15, 2012 at 09:11:07PM +0530, Rabin Vincent wrote:
> > >> On Sun, Jan 15, 2012 at 08:58:53PM +0800, Wu Fengguang wrote:
> > >> > On Sun, Jan 15, 2012 at 03:58:43PM +0530, Rabin Vincent wrote:
> > >> > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > >> > > index 71034f4..a39ad70 100644
> > >> > > --- a/mm/backing-dev.c
> > >> > > +++ b/mm/backing-dev.c
> > >> > > @@ -318,7 +318,7 @@ static void wakeup_timer_fn(unsigned long data)
> > >> > >   if (bdi->wb.task) {
> > >> > >           trace_writeback_wake_thread(bdi);
> > >> > >           wake_up_process(bdi->wb.task);
> > >> > > - } else {
> > >> > > + } else if (bdi->dev) {
> > >> > >           /*
> > >> > >            * When bdi tasks are inactive for long time, they are killed.
> > >> > >            * In this case we have to wake-up the forker thread which
> > >> > > @@ -584,6 +584,8 @@ EXPORT_SYMBOL(bdi_register_dev);
> > >> > >   */
> > >> > >  static void bdi_wb_shutdown(struct backing_dev_info *bdi)
> > >> > >  {
> > >> > > + struct task_struct *task = NULL;
> > 
> > Thanks, I fully understand.
> > In addition, Would not bdi_wakeup_flusher also be fixed?
> 
> Yes, indeed!

But wait.. Rabin's patch actually fixes the NULL deference in
the call

        trace_writeback_wake_forker_thread(bdi);

The wakeup of the forker thread should be harmless.

Thanks,
Fengguang

> > >From dc7d4e86911c0a7ea35043485b04e8a09aa74ffd Mon Sep 17 00:00:00 2001
> > From: Chanho Min <chanho.min@lge.com>
> > Date: Mon, 16 Jan 2012 14:14:48 +0900
> > Subject: [PATCH] backing-dev: add for 'fix wakeup timer races with
> > bdi_unregister()'
> > 
> > Signed-off-by: Chanho Min <chanho.min@lge.com>
> > ---
> >  fs/fs-writeback.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index f855916..ee40833 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -97,7 +97,7 @@ static void bdi_wakeup_flusher(struct backing_dev_info *bdi)
> >  {
> >         if (bdi->wb.task) {
> >                 wake_up_process(bdi->wb.task);
> > -       } else {
> > +       } else if (bdi->dev) {
> >                 /*
> >                  * The bdi thread isn't there, wake up the forker thread which
> >                  * will create and run it.
> > -- 
> > 1.7.0.4

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Chanho Min]

> But wait.. Rabin's patch actually fixes the NULL deference in
> the call
>
>        trace_writeback_wake_forker_thread(bdi);
>
> The wakeup of the forker thread should be harmless.
>
The forker thread shoud not be woken up after the bdi is unregistered.
Is this also Rabin's intention?
Also, I'm not sure the wakeup of the forker thread is harmless as bellows.
..
case FORK_THREAD:
..
  task = kthread_create(bdi_writeback_thread, &bdi->wb,
		      "flush-%s", dev_name(bdi->dev));

Thanks
Chanho

Re: [PATCH] mm/backing-dev.c: fix crash when USB/SCSI device is detached

[Rabin Vincent]

On Mon, Jan 16, 2012 at 12:04, Chanho Min <chanho0207@gmail.com> wrote:
>> But wait.. Rabin's patch actually fixes the NULL deference in
>> the call
>>
>>        trace_writeback_wake_forker_thread(bdi);
>>
>> The wakeup of the forker thread should be harmless.
>>
> The forker thread shoud not be woken up after the bdi is unregistered.
> Is this also Rabin's intention?
> Also, I'm not sure the wakeup of the forker thread is harmless as bellows.
> ..
> case FORK_THREAD:
> ..
>  task = kthread_create(bdi_writeback_thread, &bdi->wb,
>                      "flush-%s", dev_name(bdi->dev));

By the time the bdi->dev is made NULL, the bdi has been removed from the
bdi_list (in bdi_wb_shutdown()), so even if the forker thread is woken
up after that it will not find the bdi and won't get to this part of the
code.