Messages in this thread | | | Subject | Re: [PATCH] ext4: avoid huge mmp update interval value | From | Pavel Skripkin <> | Date | Fri, 6 Aug 2021 13:20:29 +0300 |
| |
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
| |