lkml.org 
[lkml]   [2021]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] ext4: avoid huge mmp update interval value
From
Date
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

\
 
 \ /
  Last update: 2021-08-06 12:21    [W:0.169 / U:0.364 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site