linux-ext4 - Re: [BUG] ext4: cannot unfreeze a filesystem due to a deadlock

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: <4D5C9B1B.2050304@jp.fujitsu.com>
Date:	Thu, 17 Feb 2011 12:50:51 +0900
From:	Toshiyuki Okajima <toshi.okajima@...fujitsu.com>
To:	Jan Kara <jack@...e.cz>
CC:	Ted Ts'o <tytso@....edu>,
	Masayoshi MIZUMA <m.mizuma@...fujitsu.com>,
	Andreas Dilger <adilger.kernel@...ger.ca>,
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [BUG] ext4: cannot unfreeze a filesystem due to a deadlock

(2011/02/16 23:56), Jan Kara wrote:
> On Wed 16-02-11 08:17:46, Toshiyuki Okajima wrote:
>> On Tue, 15 Feb 2011 18:29:54 +0100
>> Jan Kara<jack@...e.cz>  wrote:
>>> On Tue 15-02-11 12:03:52, Ted Ts'o wrote:
>>>> On Tue, Feb 15, 2011 at 05:06:30PM +0100, Jan Kara wrote:
>>>>> Thanks for detailed analysis. Indeed this is a bug. Whenever we do IO
>>>>> under s_umount semaphore, we are prone to deadlock like the one you
>>>>> describe above.
>>>>
>>>> One of the fundamental problems here is that the freeze and thaw
>>>> routines are using down_write(&sb->s_umount) for two purposes.  The
>>>> first is to prevent the resume/thaw from racing with a umount (which
>>>> it could do just as well by taking a read lock), but the second is to
>>>> prevent the resume/thaw code from racing with itself.  That's the core
>>>> fundamental problem here.
>>>>
>>>> So I think we can solve this by introduce a new mutex, s_freeze, and
>>>> having the the resume/thaw first take the s_freeze mutex and then
>>>> second take a read lock on the s_umount.
>>>    Sadly this does not quite work because even down_read(&sb->s_umount)
>>> in thaw_super() can block if there is another process that tries to acquire
>>> s_umount for writing - a situation like:
>>>    TASK 1 (e.g. flusher)		TASK 2	(e.g. remount)		TASK 3 (unfreeze)
>>> down_read(&sb->s_umount)
>>>    block on s_frozen
>>> 				down_write(&sb->s_umount)
>>> 				  -blocked
>>> 								down_read(&sb->s_umount)
>>> 								  -blocked
>>> behind the write access...
>>>
>>> The only working solution I see is to check for frozen filesystem before
>>> taking s_umount semaphore which seems rather ugly (but might be bearable if
>>> we did so in some well described wrapper).
>> I created the patch that you imagine yesterday.
>>
>> I got a reproducer from Mizuma-san yesterday, and then I executed it on the kernel
>> without a fixed patch. After an hour, I confirmed that this deadlock happened.
>>
>> However, on the kernel with a fixed patch, this deadlock doesn't still happen
>> after 12 hours passed.
>>
>> The patch for linux-2.6.38-rc4 is as follows:
>> ---
>>   fs/fs-writeback.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
>> index 59c6e49..1c9a05e 100644
>> --- a/fs/fs-writeback.c
>> +++ b/fs/fs-writeback.c
>> @@ -456,7 +456,7 @@ static bool pin_sb_for_writeback(struct super_block *sb)
>>          spin_unlock(&sb_lock);
>>
>>          if (down_read_trylock(&sb->s_umount)) {
>> -               if (sb->s_root)
>> +               if (sb->s_frozen == SB_UNFROZEN&&  sb->s_root)
>>                          return true;
>>                  up_read(&sb->s_umount);

>    So this is something along the lines I thought but it actually won't work
> for example if sync(1) is run while the filesystem is frozen (that takes
> s_umount semaphore in a different place). And generally, I'm not convinced
> there are not other places that try to do IO while holding s_umount
> semaphore...
OK. I understand.

This code only fixes the case for the following path:
writeback_inodes_wb
-> ext4_da_writepages
    -> ext4_journal_start_sb
       -> vfs_check_frozen
But, the code doesn't fix the other cases.

We must modify the local filesystem part in order to fix all cases...?

Regards,
Toshiyuki Okajima

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html