linux-ext4 - Re: [PATCH] ext4: avoid huge mmp update interval value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210805225926.GB2566745@dread.disaster.area>
Date:   Fri, 6 Aug 2021 08:59:26 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     Theodore Ts'o <tytso@....edu>, adilger.kernel@...ger.ca,
        johann@...mcloud.com, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot+c9ff4822a62eee994ea3@...kaller.appspotmail.com
Subject: Re: [PATCH] ext4: avoid huge mmp update interval value

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....

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com