On 8/6/21 1:59 AM, Dave Chinner wrote:
On Thu, Aug 05, 2021 at 11:12:42PM +0300, Pavel Skripkin wrote:
On 8/5/21 10:45 PM, Theodore Ts'o wrote:
> On Thu, Aug 05, 2021 at 06:14:18PM +0300, Pavel Skripkin wrote:
> > Syzbot reported task hung bug in ext4_fill_super(). The problem was in
> > too huge mmp update interval.
> >
> > Syzkaller reproducer setted s_mmp_update_interval to 39785 seconds. This
> > update interaval is unreasonable huge and it can cause tasks to hung on
> > kthread_stop() call, since it will wait until timeout timer expires.
>
> I must be missing something. kthread_stop() should wake up the
> kmmpd() thread, which should see kthread_should_stop(), and then it
> should exit. What is causing it to wait until the timeout timer
> expires?
>
> - Ted
>
Hi, Ted!
I guess, I've explained my idea badly, sorry :)
I mean, that there is a chance to hit this situation:
CPU0 CPU1
kthread_should_stop() <-- false
kthread_stop()
set_bit(KTHREAD_SHOULD_STOP)
wake_up_process()
wait_for_completion()
schedule_timeout_interruptible()
*waits until timer expires*
Yeah, so the bug here is checking kthread_should_stop() while
the task state is TASK_RUNNING.
What you need to do here is:
while (run) {
....
set_current_state(TASK_INTERRUPTIBLE);
if (kthread_should_stop()) {
__set_current_state(TASK_RUNNING);
break;
}
schedule_timeout(tout);
.....
}
That means in the case above where schedule() occurs after the
kthread_should_stop() check has raced with kthread_stop(), then
wake_up_process() will handle any races with schedule() correctly.
i.e. wake_up_process() will set the task state to TASK_RUNNING and
schedule() will not sleep if it is called after wake_up_process().
Or if schedule() runs first then wake_up_process() will wake it
correctly after setting the state to TASK_RUNNING.
Either way, the loop then runs around again straight away to the next
kthread_should_stop() call, at which point it breaks out.
I note that the "wait_to_exit:" code in the same function does this
properly....
Hi, Dave!
I've tested your suggestion with syzbot and it works, thank you!
Anyway, @Ted, does it make sense to add boundaries for
s_mmp_update_interval? I think, too big update interval for mmp isn't
reasonable. I can send patch series with Dave's suggestion and previous
patch. What do you think?
With regards,
Pavel Skripkin
