Re: [PATCH] kernel:time fix race condition in alaramtimer.c
Ok you're right, that prevents it. Sorry, I missed this check somehow. Cheers Marcus On 07/08/2013 07:31 AM, Srivatsa S. Bhat wrote: > On 07/06/2013 09:37 PM, Marcus Gelderie wrote: >> This patch fixes a race condition whereby the process can be caused to >> sleep indefinitely. The problem occurs when the process is preempted >> after having set its state to TASK_INTERRUPTIBLE but before starting the >> alarm. >> > > I don't think there is any such problem. The __schedule() function does > this check before dequeuing a task: > > if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) > > The latter part helps prevent the kind of indefinite sleeping you described > above. Also, take a look at preempt_schedule(), preempt_schedule_irq() > and friends to see how in-kernel preemption is dealt with, by using > PREEMPT_ACTIVE. That would clarify why there is no bug here. > > Regards, > Srivatsa S. Bhat > >> --- >> kernel/time/alarmtimer.c | 6 +- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c >> index f11d83b..81c8b31 100644 >> --- a/kernel/time/alarmtimer.c >> +++ b/kernel/time/alarmtimer.c >> @@ -585,15 +585,19 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, >> ktime_t absexp) >> { >> alarm->data = (void *)current; >> do { >> +preempt_disable(); >> set_current_state(TASK_INTERRUPTIBLE); >> alarm_start(alarm, absexp); >> +preempt_enable_no_resched(); >> if (likely(alarm->data)) >> schedule(); >> >> +preempt_disable(); >> alarm_cancel(alarm); >> +__set_current_state(TASK_RUNNING); >> +preempt_enable_no_resched(); >> } while (alarm->data && !signal_pending(current)); >> >> -__set_current_state(TASK_RUNNING); >> >> return (alarm->data == NULL); >> } >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel:time fix race condition in alaramtimer.c
Ok you're right, that prevents it. Sorry, I missed this check somehow. Cheers Marcus On 07/08/2013 07:31 AM, Srivatsa S. Bhat wrote: On 07/06/2013 09:37 PM, Marcus Gelderie wrote: This patch fixes a race condition whereby the process can be caused to sleep indefinitely. The problem occurs when the process is preempted after having set its state to TASK_INTERRUPTIBLE but before starting the alarm. I don't think there is any such problem. The __schedule() function does this check before dequeuing a task: if (prev-state !(preempt_count() PREEMPT_ACTIVE)) The latter part helps prevent the kind of indefinite sleeping you described above. Also, take a look at preempt_schedule(), preempt_schedule_irq() and friends to see how in-kernel preemption is dealt with, by using PREEMPT_ACTIVE. That would clarify why there is no bug here. Regards, Srivatsa S. Bhat --- kernel/time/alarmtimer.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index f11d83b..81c8b31 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -585,15 +585,19 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp) { alarm-data = (void *)current; do { +preempt_disable(); set_current_state(TASK_INTERRUPTIBLE); alarm_start(alarm, absexp); +preempt_enable_no_resched(); if (likely(alarm-data)) schedule(); +preempt_disable(); alarm_cancel(alarm); +__set_current_state(TASK_RUNNING); +preempt_enable_no_resched(); } while (alarm-data !signal_pending(current)); -__set_current_state(TASK_RUNNING); return (alarm-data == NULL); } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel:time fix race condition in alaramtimer.c
On 07/06/2013 09:37 PM, Marcus Gelderie wrote: > This patch fixes a race condition whereby the process can be caused to > sleep indefinitely. The problem occurs when the process is preempted > after having set its state to TASK_INTERRUPTIBLE but before starting the > alarm. > I don't think there is any such problem. The __schedule() function does this check before dequeuing a task: if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) The latter part helps prevent the kind of indefinite sleeping you described above. Also, take a look at preempt_schedule(), preempt_schedule_irq() and friends to see how in-kernel preemption is dealt with, by using PREEMPT_ACTIVE. That would clarify why there is no bug here. Regards, Srivatsa S. Bhat > --- > kernel/time/alarmtimer.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c > index f11d83b..81c8b31 100644 > --- a/kernel/time/alarmtimer.c > +++ b/kernel/time/alarmtimer.c > @@ -585,15 +585,19 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, > ktime_t absexp) > { > alarm->data = (void *)current; > do { > + preempt_disable(); > set_current_state(TASK_INTERRUPTIBLE); > alarm_start(alarm, absexp); > + preempt_enable_no_resched(); > if (likely(alarm->data)) > schedule(); > > + preempt_disable(); > alarm_cancel(alarm); > + __set_current_state(TASK_RUNNING); > + preempt_enable_no_resched(); > } while (alarm->data && !signal_pending(current)); > > - __set_current_state(TASK_RUNNING); > > return (alarm->data == NULL); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel:time fix race condition in alaramtimer.c
On 07/06/2013 09:37 PM, Marcus Gelderie wrote: This patch fixes a race condition whereby the process can be caused to sleep indefinitely. The problem occurs when the process is preempted after having set its state to TASK_INTERRUPTIBLE but before starting the alarm. I don't think there is any such problem. The __schedule() function does this check before dequeuing a task: if (prev-state !(preempt_count() PREEMPT_ACTIVE)) The latter part helps prevent the kind of indefinite sleeping you described above. Also, take a look at preempt_schedule(), preempt_schedule_irq() and friends to see how in-kernel preemption is dealt with, by using PREEMPT_ACTIVE. That would clarify why there is no bug here. Regards, Srivatsa S. Bhat --- kernel/time/alarmtimer.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index f11d83b..81c8b31 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -585,15 +585,19 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp) { alarm-data = (void *)current; do { + preempt_disable(); set_current_state(TASK_INTERRUPTIBLE); alarm_start(alarm, absexp); + preempt_enable_no_resched(); if (likely(alarm-data)) schedule(); + preempt_disable(); alarm_cancel(alarm); + __set_current_state(TASK_RUNNING); + preempt_enable_no_resched(); } while (alarm-data !signal_pending(current)); - __set_current_state(TASK_RUNNING); return (alarm-data == NULL); } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] kernel:time fix race condition in alaramtimer.c
This patch fixes a race condition whereby the process can be caused to sleep indefinitely. The problem occurs when the process is preempted after having set its state to TASK_INTERRUPTIBLE but before starting the alarm. Disabling preemption until the alarm has been set solves this problem. A second problem may occur if the process reaches the alarm_cancel call while still being in TASK_INTERRUPTIBLE. I do not see how this could possibly happen (if schedule returns, the state is TASK_RUNNING; if schedule is never called, the wakeup callback must have woken the process (data==NULL) thereby setting its state to TASK_RUNNING) but the code is clearly written under the assumption that it can happen. In this case, the state must be set to TASK_RUNNING after the alarm has been canceled for the same reasons as above. Signed-off-by: Marcus Gelderie --- kernel/time/alarmtimer.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index f11d83b..81c8b31 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -585,15 +585,19 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp) { alarm->data = (void *)current; do { + preempt_disable(); set_current_state(TASK_INTERRUPTIBLE); alarm_start(alarm, absexp); + preempt_enable_no_resched(); if (likely(alarm->data)) schedule(); + preempt_disable(); alarm_cancel(alarm); + __set_current_state(TASK_RUNNING); + preempt_enable_no_resched(); } while (alarm->data && !signal_pending(current)); - __set_current_state(TASK_RUNNING); return (alarm->data == NULL); } -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] kernel:time fix race condition in alaramtimer.c
This patch fixes a race condition whereby the process can be caused to sleep indefinitely. The problem occurs when the process is preempted after having set its state to TASK_INTERRUPTIBLE but before starting the alarm. Disabling preemption until the alarm has been set solves this problem. A second problem may occur if the process reaches the alarm_cancel call while still being in TASK_INTERRUPTIBLE. I do not see how this could possibly happen (if schedule returns, the state is TASK_RUNNING; if schedule is never called, the wakeup callback must have woken the process (data==NULL) thereby setting its state to TASK_RUNNING) but the code is clearly written under the assumption that it can happen. In this case, the state must be set to TASK_RUNNING after the alarm has been canceled for the same reasons as above. Signed-off-by: Marcus Gelderie redm...@gmail.com --- kernel/time/alarmtimer.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index f11d83b..81c8b31 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -585,15 +585,19 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp) { alarm-data = (void *)current; do { + preempt_disable(); set_current_state(TASK_INTERRUPTIBLE); alarm_start(alarm, absexp); + preempt_enable_no_resched(); if (likely(alarm-data)) schedule(); + preempt_disable(); alarm_cancel(alarm); + __set_current_state(TASK_RUNNING); + preempt_enable_no_resched(); } while (alarm-data !signal_pending(current)); - __set_current_state(TASK_RUNNING); return (alarm-data == NULL); } -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/