linux-ext4 - Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths.

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] [day] [month] [year] [list]

Message-ID: <CAE0An6eJmfK0bZ-6pTXbxxx8CXVx1JMh_FnCvCDmOc97rNndcw@mail.gmail.com>
Date:	Sun, 10 Jul 2011 12:11:52 -0700
From:	Manish Katiyar <mkatiyar@...il.com>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	"Theodore Ts'o" <tytso@....edu>, ext4 <linux-ext4@...r.kernel.org>,
	Yu Jian <yujian@...mcloud.com>
Subject: Re: [PATCH] ext4: Fix missing iput for root inode in case of all
 failed mount paths.

On Wed, Jul 6, 2011 at 3:54 PM, Andreas Dilger <adilger@...ger.ca> wrote:
> On 2011-01-16, at 11:05 AM, Manish Katiyar wrote:
>> On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <adilger@...ger.ca> wrote:
>>> Why not just put the iput() at failed_mount4() instead of spread around the code?
>>
>> Thanks Andreas, Here is the updated patch.
>
> We are hitting this same problem due to ENOMEM on allocating some large
> filesystem structures for 128TB filesystems.  However, when we were going
> to add this patch to our patch series (until vendor kernels include it),
> Yu Jian (one of our developers) noticed a problem with the patch.
>
> In the error path, the patch is doing:
>
> failed_mount4:
>        iput(root);
>        sb->s_root = NULL;
>
> but in fact sb->s_root is a dentry allocated by d_alloc_root(), so the
> above code is freeing the inode, but still leaking the dentry.  This
> is of course a lot better than before (no oops), but still isn't correct.
>
> The original problem appears to have been inadvertently fixed with commit
> 8aefcd557d26d0023a36f9ec5afbf55e59f8f26b, because ext4_clear_inode() now
> checks "if (EXT4_I(inode)->jinode)" before deferencing EXT4_SB() and the
> now-NULL s_fs_info.  jinode should be NULL during mount, because it has
> never been opened.  I haven't confirmed this theory yet, however.  Manish,
> can you please give this a try with your fault-injection testing?
>
> It looks like we could revert 32a9bb57d7c1fd04ae0f72b8f671501f000a0e9f
> (this patch, leaving the two explicit iput() in place in case of a bad
> root inode or dentry) and leave the VFS to clean up the root dentry.

Hi Andreas,

I reverted my original patch in latest Ted's tree and retried.
ext4_clear_inode() is fixed, but we still
panic in

#14 <signal handler called>
#15 ext4_sync_fs (sb=0x17d6ee00, wait=0) at fs/ext4/super.c:4191
#16 0x080d5214 in __sync_filesystem (sb=0x17d6ee00, wait=0) at fs/sync.c:49
#17 0x080d5277 in sync_filesystem (sb=0x17d6ee00) at fs/sync.c:74
#18 0x080bc188 in generic_shutdown_super (sb=0x17d6ee00) at fs/super.c:282
#19 0x080bc236 in kill_block_super (sb=0x17d6ee00) at fs/super.c:856
#20 0x080bc3fe in deactivate_locked_super (s=0x17d6ee00) at fs/super.c:183
#21 0x080bc96b in mount_bdev (fs_type=0x8264974, flags=0,
dev_name=0x17d74ed0 "/dev/loop0", data=0x0, fill_super=0x81236f1
<ext4_fill_super>) at fs/super.c:831
#22 0x0811fc36 in ext4_mount (fs_type=0x8264974, flags=0,
dev_name=0x17d74ed0 "/dev/loop0", data=0x0) at fs/ext4/super.c:4820
(gdb) f 15
#15 ext4_sync_fs (sb=0x17d6ee00, wait=0) at fs/ext4/super.c:4191
4191		flush_workqueue(sbi->dio_unwritten_wq);
(gdb) p sbi
$1 = (struct ext4_sb_info *) 0x0


-- 
Thanks -
Manish
--
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