Messages in this thread | | | Date | Fri, 2 Sep 2011 10:47:03 +0530 | Subject | Re: [PATCH 1/1] mm/backing-dev.c: Call del_timer_sync instead of del_timer | From | "kautuk.c @samsung.com" <> |
| |
Hi,
On Fri, Sep 2, 2011 at 3:03 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 1 Sep 2011 21:27:02 +0530 > Kautuk Consul <consul.kautuk@gmail.com> wrote: > >> This is important for SMP scenario, to check whether the timer >> callback is executing on another CPU when we are deleting the >> timer. >> > > I don't see why? > >> index d6edf8d..754b35a 100644 >> --- a/mm/backing-dev.c >> +++ b/mm/backing-dev.c >> @@ -385,7 +385,7 @@ static int bdi_forker_thread(void *ptr) >> * dirty data on the default backing_dev_info >> */ >> if (wb_has_dirty_io(me) || !list_empty(&me->bdi->work_list)) { >> - del_timer(&me->wakeup_timer); >> + del_timer_sync(&me->wakeup_timer); >> wb_do_writeback(me, 0); >> } > > It isn't a use-after-free fix: bdi_unregister() safely shoots down any > running timer. >
In the situation that we do a del_timer at the same time that the wakeup_timer_fn is executing on another CPU, there is one tiny possible problem: 1) The wakeup_timer_fn will call wake_up_process on the bdi-default thread. This will set the bdi-default thread's state to TASK_RUNNING. 2) However, the code in bdi_writeback_thread() sets the state of the bdi-default process to TASK_INTERRUPTIBLE as it intends to sleep later.
If 2) happens before 1), then the bdi_forker_thread will not sleep inside schedule as is the intention of the bdi_forker_thread() code.
This protection is not achieved even by acquiring spinlocks before setting the task->state as the spinlock used in wakeup_timer_fn is &bdi->wb_lock whereas the code in bdi_forker_thread acquires &bdi_lock which is a different spin_lock.
Am I correct in concluding this ?
> Please completely explain what you believe the problem is here. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |