Fix missing iput for root inode in case of all failed mount paths.
Fixes bug#26752
Signed-off-by: Manish Katiyar <[email protected]>
---
fs/ext4/super.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cb10a06..9570fcc 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3587,6 +3587,7 @@ no_journal:
if (err) {
ext4_msg(sb, KERN_ERR, "failed to initialize system "
"zone (%d)", err);
+ iput(root);
goto failed_mount4;
}
@@ -3595,12 +3596,15 @@ no_journal:
if (err) {
ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)",
err);
+ iput(root);
goto failed_mount4;
}
err = ext4_register_li_request(sb, first_not_zeroed);
- if (err)
+ if (err) {
+ iput(root);
goto failed_mount4;
+ }
sbi->s_kobj.kset = ext4_kset;
init_completion(&sbi->s_kobj_unregister);
@@ -3609,6 +3613,7 @@ no_journal:
if (err) {
ext4_mb_release(sb);
ext4_ext_release(sb);
+ iput(root);
goto failed_mount4;
};
@@ -3648,6 +3653,7 @@ cantfind_ext4:
goto failed_mount;
failed_mount4:
+ sb->s_root = NULL;
ext4_msg(sb, KERN_ERR, "mount failed");
destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
failed_mount_wq:
--
1.7.1
--
Thanks -
Manish
==================================
[$\*.^ -- I miss being one of them
==================================
Why not just put the iput() at failed_mount4() instead of spread around the code?
Cheers, Andreas
On 2011-01-16, at 0:30, Manish Katiyar <[email protected]> wrote:
> Fix missing iput for root inode in case of all failed mount paths.
> Fixes bug#26752
>
> Signed-off-by: Manish Katiyar <[email protected]>
>
> ---
> fs/ext4/super.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cb10a06..9570fcc 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3587,6 +3587,7 @@ no_journal:
> if (err) {
> ext4_msg(sb, KERN_ERR, "failed to initialize system "
> "zone (%d)", err);
> + iput(root);
> goto failed_mount4;
> }
>
> @@ -3595,12 +3596,15 @@ no_journal:
> if (err) {
> ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)",
> err);
> + iput(root);
> goto failed_mount4;
> }
>
> err = ext4_register_li_request(sb, first_not_zeroed);
> - if (err)
> + if (err) {
> + iput(root);
> goto failed_mount4;
> + }
>
> sbi->s_kobj.kset = ext4_kset;
> init_completion(&sbi->s_kobj_unregister);
> @@ -3609,6 +3613,7 @@ no_journal:
> if (err) {
> ext4_mb_release(sb);
> ext4_ext_release(sb);
> + iput(root);
> goto failed_mount4;
> };
>
> @@ -3648,6 +3653,7 @@ cantfind_ext4:
> goto failed_mount;
>
> failed_mount4:
> + sb->s_root = NULL;
> ext4_msg(sb, KERN_ERR, "mount failed");
> destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
> failed_mount_wq:
> --
> 1.7.1
>
>
> --
> Thanks -
> Manish
> ==================================
> [$\*.^ -- I miss being one of them
> ==================================
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <[email protected]> wrote:
> Why not just put the iput() at failed_mount4() instead of spread around the code?
Thanks Andreas, Here is the updated patch.
Signed-off-by: Manish Katiyar <[email protected]>
---
fs/ext4/super.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cb10a06..8fa53e9 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3522,17 +3522,16 @@ no_journal:
if (IS_ERR(root)) {
ext4_msg(sb, KERN_ERR, "get root inode failed");
ret = PTR_ERR(root);
+ root = NULL;
goto failed_mount4;
}
if (!S_ISDIR(root->i_mode) || !root->i_blocks || !root->i_size) {
- iput(root);
ext4_msg(sb, KERN_ERR, "corrupt root inode, run e2fsck");
goto failed_mount4;
}
sb->s_root = d_alloc_root(root);
if (!sb->s_root) {
ext4_msg(sb, KERN_ERR, "get root dentry failed");
- iput(root);
ret = -ENOMEM;
goto failed_mount4;
}
@@ -3648,6 +3647,8 @@ cantfind_ext4:
goto failed_mount;
failed_mount4:
+ iput(root);
+ sb->s_root = NULL;
ext4_msg(sb, KERN_ERR, "mount failed");
destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
failed_mount_wq:
--
1.7.1
>
> Cheers, Andreas
>
> On 2011-01-16, at 0:30, Manish Katiyar <[email protected]> wrote:
>
>> Fix missing iput for root inode in case of all failed mount paths.
>> Fixes bug#26752
>>
>> Signed-off-by: Manish Katiyar <[email protected]>
>>
>> ---
>> fs/ext4/super.c | ? ?8 +++++++-
>> 1 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index cb10a06..9570fcc 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -3587,6 +3587,7 @@ no_journal:
>> ? ? ? ?if (err) {
>> ? ? ? ? ? ? ? ?ext4_msg(sb, KERN_ERR, "failed to initialize system "
>> ? ? ? ? ? ? ? ? ? ? ? ? "zone (%d)", err);
>> + ? ? ? ? ? ? ? iput(root);
>> ? ? ? ? ? ? ? ?goto failed_mount4;
>> ? ? ? ?}
>>
>> @@ -3595,12 +3596,15 @@ no_journal:
>> ? ? ? ?if (err) {
>> ? ? ? ? ? ? ? ?ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)",
>> ? ? ? ? ? ? ? ? ? ? ? ? err);
>> + ? ? ? ? ? ? ? iput(root);
>> ? ? ? ? ? ? ? ?goto failed_mount4;
>> ? ? ? ?}
>>
>> ? ? ? ?err = ext4_register_li_request(sb, first_not_zeroed);
>> - ? ? ? if (err)
>> + ? ? ? if (err) {
>> + ? ? ? ? ? ? ? iput(root);
>> ? ? ? ? ? ? ? ?goto failed_mount4;
>> + ? ? ? }
>>
>> ? ? ? ?sbi->s_kobj.kset = ext4_kset;
>> ? ? ? ?init_completion(&sbi->s_kobj_unregister);
>> @@ -3609,6 +3613,7 @@ no_journal:
>> ? ? ? ?if (err) {
>> ? ? ? ? ? ? ? ?ext4_mb_release(sb);
>> ? ? ? ? ? ? ? ?ext4_ext_release(sb);
>> + ? ? ? ? ? ? ? iput(root);
>> ? ? ? ? ? ? ? ?goto failed_mount4;
>> ? ? ? ?};
>>
>> @@ -3648,6 +3653,7 @@ cantfind_ext4:
>> ? ? ? ?goto failed_mount;
>>
>> failed_mount4:
>> + ? ? ? sb->s_root = NULL;
>> ? ? ? ?ext4_msg(sb, KERN_ERR, "mount failed");
>> ? ? ? ?destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
>> failed_mount_wq:
>> --
>> 1.7.1
>>
>>
>> --
>> Thanks -
>> Manish
>> ==================================
>> [$\*.^ -- I miss being one of them
>> ==================================
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
--
Thanks -
Manish
==================================
[$\*.^ -- I miss being one of them
==================================
On Sun, Jan 16, 2011 at 10:05 AM, Manish Katiyar <[email protected]> wrote:
> On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <[email protected]> wrote:
>> Why not just put the iput() at failed_mount4() instead of spread around the code?
>
> Thanks Andreas, Here is the updated patch.
>
> Signed-off-by: Manish Katiyar <[email protected]>
> ---
> ?fs/ext4/super.c | ? ?5 +++--
> ?1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cb10a06..8fa53e9 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3522,17 +3522,16 @@ no_journal:
> ? ? ? ?if (IS_ERR(root)) {
> ? ? ? ? ? ? ? ?ext4_msg(sb, KERN_ERR, "get root inode failed");
> ? ? ? ? ? ? ? ?ret = PTR_ERR(root);
> + ? ? ? ? ? ? ? root = NULL;
> ? ? ? ? ? ? ? ?goto failed_mount4;
> ? ? ? ?}
> ? ? ? ?if (!S_ISDIR(root->i_mode) || !root->i_blocks || !root->i_size) {
> - ? ? ? ? ? ? ? iput(root);
> ? ? ? ? ? ? ? ?ext4_msg(sb, KERN_ERR, "corrupt root inode, run e2fsck");
> ? ? ? ? ? ? ? ?goto failed_mount4;
> ? ? ? ?}
> ? ? ? ?sb->s_root = d_alloc_root(root);
> ? ? ? ?if (!sb->s_root) {
> ? ? ? ? ? ? ? ?ext4_msg(sb, KERN_ERR, "get root dentry failed");
> - ? ? ? ? ? ? ? iput(root);
> ? ? ? ? ? ? ? ?ret = -ENOMEM;
> ? ? ? ? ? ? ? ?goto failed_mount4;
> ? ? ? ?}
> @@ -3648,6 +3647,8 @@ cantfind_ext4:
> ? ? ? ?goto failed_mount;
>
> ?failed_mount4:
> + ? ? ? iput(root);
> + ? ? ? sb->s_root = NULL;
> ? ? ? ?ext4_msg(sb, KERN_ERR, "mount failed");
> ? ? ? ?destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
> ?failed_mount_wq:
> --
> 1.7.1
>
>
>
>>
>> Cheers, Andreas
>>
>> On 2011-01-16, at 0:30, Manish Katiyar <[email protected]> wrote:
>>
>>> Fix missing iput for root inode in case of all failed mount paths.
>>> Fixes bug#26752
>>>
>>> Signed-off-by: Manish Katiyar <[email protected]>
>>>
>>> ---
>>> fs/ext4/super.c | ? ?8 +++++++-
>>> 1 files changed, 7 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> index cb10a06..9570fcc 100644
>>> --- a/fs/ext4/super.c
>>> +++ b/fs/ext4/super.c
>>> @@ -3587,6 +3587,7 @@ no_journal:
>>> ? ? ? ?if (err) {
>>> ? ? ? ? ? ? ? ?ext4_msg(sb, KERN_ERR, "failed to initialize system "
>>> ? ? ? ? ? ? ? ? ? ? ? ? "zone (%d)", err);
>>> + ? ? ? ? ? ? ? iput(root);
>>> ? ? ? ? ? ? ? ?goto failed_mount4;
>>> ? ? ? ?}
>>>
>>> @@ -3595,12 +3596,15 @@ no_journal:
>>> ? ? ? ?if (err) {
>>> ? ? ? ? ? ? ? ?ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)",
>>> ? ? ? ? ? ? ? ? ? ? ? ? err);
>>> + ? ? ? ? ? ? ? iput(root);
>>> ? ? ? ? ? ? ? ?goto failed_mount4;
>>> ? ? ? ?}
>>>
>>> ? ? ? ?err = ext4_register_li_request(sb, first_not_zeroed);
>>> - ? ? ? if (err)
>>> + ? ? ? if (err) {
>>> + ? ? ? ? ? ? ? iput(root);
>>> ? ? ? ? ? ? ? ?goto failed_mount4;
>>> + ? ? ? }
>>>
>>> ? ? ? ?sbi->s_kobj.kset = ext4_kset;
>>> ? ? ? ?init_completion(&sbi->s_kobj_unregister);
>>> @@ -3609,6 +3613,7 @@ no_journal:
>>> ? ? ? ?if (err) {
>>> ? ? ? ? ? ? ? ?ext4_mb_release(sb);
>>> ? ? ? ? ? ? ? ?ext4_ext_release(sb);
>>> + ? ? ? ? ? ? ? iput(root);
>>> ? ? ? ? ? ? ? ?goto failed_mount4;
>>> ? ? ? ?};
>>>
>>> @@ -3648,6 +3653,7 @@ cantfind_ext4:
>>> ? ? ? ?goto failed_mount;
>>>
>>> failed_mount4:
>>> + ? ? ? sb->s_root = NULL;
>>> ? ? ? ?ext4_msg(sb, KERN_ERR, "mount failed");
>>> ? ? ? ?destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
>>> failed_mount_wq:
Hi Ted,
Any feedback on this ?
--
Thanks -
Manish
On Sat, Jan 29, 2011 at 09:40:34PM -0800, Manish Katiyar wrote:
>
> Any feedback on this ?
The patch looks good, but it doesn't fix bug #26752.
The root cause of that bug is the fact that ext4_sync_fs() and
ext4_put_super() assumes that sbi has been initialized.
- Ted
On Mon, Feb 7, 2011 at 3:23 PM, Ted Ts'o <[email protected]> wrote:
> On Sat, Jan 29, 2011 at 09:40:34PM -0800, Manish Katiyar wrote:
>>
>> Any feedback on this ?
>
> The patch looks good, but it doesn't fix bug #26752.
>
> The root cause of that bug is the fact that ext4_sync_fs() and
> ext4_put_super() assumes that sbi has been initialized.
Hmm.. Actually I was looking at generic_super_shutdown() which I
thought would call ext4_put_super() only if sb->root is set, and since
I was setting sb->root explicitly to NULL in my patch in case of
failures.
--
Thanks -
Manish
On Mon, Feb 7, 2011 at 3:31 PM, Manish Katiyar <[email protected]> wrote:
> On Mon, Feb 7, 2011 at 3:23 PM, Ted Ts'o <[email protected]> wrote:
>> On Sat, Jan 29, 2011 at 09:40:34PM -0800, Manish Katiyar wrote:
>>>
>>> Any feedback on this ?
>>
>> The patch looks good, but it doesn't fix bug #26752.
>>
>> The root cause of that bug is the fact that ext4_sync_fs() and
>> ext4_put_super() assumes that sbi has been initialized.
Hi Ted,
Is there anything else that I need to do to get this merged in your tree ?
>
> Hmm.. Actually I was looking at generic_super_shutdown() which I
> thought would call ext4_put_super() only if sb->root is set, and since
> I was setting sb->root explicitly to NULL in my patch in case of
> failures.
>
> --
> Thanks -
> Manish
>
--
Thanks -
Manish
On Sat, Feb 26, 2011 at 01:35:05PM -0800, Manish Katiyar wrote:
>
> Is there anything else that I need to do to get this merged in your tree ?
>
I've added the patch with the following explanation:
ext4: fix missing iput of root inode for some mount error paths
This assures that the root inode is not leaked, and that sb->s_root is
NULL, which will prevent generic_shutdown_super() from doing extra
work, including call sync_filesystem, which ultimately results in
ext4_sync_fs() getting called with an uninitialized struct super,
which is the cause of the crash noted in Kernel Bugzilla #26752.
https://bugzilla.kernel.org/show_bug.cgi?id=26752
Signed-off-by: Manish Katiyar <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
Sorry for the delay, but this patch was held up pending my tracingg
through the code and understanding why this really fixed BZ #26752.
In the future, adding a bit more detail in the commit log will help me
process the patch faster, since I won't have to reproduce your
analysis.
Also, just quoting a BZ number without going into more detail risks my
putting it the patch on my "to analyze --- when I have access to
network" queue if I happen to come across it while on an airplane. :-)
- Ted
On Sun, Feb 27, 2011 at 5:45 PM, Ted Ts'o <[email protected]> wrote:
> On Sat, Feb 26, 2011 at 01:35:05PM -0800, Manish Katiyar wrote:
>>
>> Is there anything else that I need to do to get this merged in your tree ?
>>
>
> I've added the patch with the following explanation:
>
> ext4: fix missing iput of root inode for some mount error paths
>
> This assures that the root inode is not leaked, and that sb->s_root is
> NULL, which will prevent generic_shutdown_super() from doing extra
> work, including call sync_filesystem, which ultimately results in
> ext4_sync_fs() getting called with an uninitialized struct super,
> which is the cause of the crash noted in Kernel Bugzilla #26752.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=26752
>
> Signed-off-by: Manish Katiyar <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
>
>
> Sorry for the delay, but this patch was held up pending my tracingg
> through the code and understanding why this really fixed BZ #26752.
> In the future, adding a bit more detail in the commit log will help me
> process the patch faster, since I won't have to reproduce your
> analysis.
>
> Also, just quoting a BZ number without going into more detail risks my
> putting it the patch on my "to analyze --- when I have access to
> network" queue if I happen to come across it while on an airplane. ?:-)
Hi Ted,
Thanks a lot. That was my first bugfix :-), will keep that in mind in future.
--
Thanks -
Manish
On 2011-01-16, at 11:05 AM, Manish Katiyar wrote:
> On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <[email protected]> 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.
> Signed-off-by: Manish Katiyar <[email protected]>
> ---
> fs/ext4/super.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cb10a06..8fa53e9 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3522,17 +3522,16 @@ no_journal:
> if (IS_ERR(root)) {
> ext4_msg(sb, KERN_ERR, "get root inode failed");
> ret = PTR_ERR(root);
> + root = NULL;
> goto failed_mount4;
> }
> if (!S_ISDIR(root->i_mode) || !root->i_blocks || !root->i_size) {
> - iput(root);
> ext4_msg(sb, KERN_ERR, "corrupt root inode, run e2fsck");
> goto failed_mount4;
> }
> sb->s_root = d_alloc_root(root);
> if (!sb->s_root) {
> ext4_msg(sb, KERN_ERR, "get root dentry failed");
> - iput(root);
> ret = -ENOMEM;
> goto failed_mount4;
> }
> @@ -3648,6 +3647,8 @@ cantfind_ext4:
> goto failed_mount;
>
> failed_mount4:
> + iput(root);
> + sb->s_root = NULL;
> ext4_msg(sb, KERN_ERR, "mount failed");
> destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
> failed_mount_wq:
> --
> 1.7.1
>
>
>
>>
>> Cheers, Andreas
>>
>> On 2011-01-16, at 0:30, Manish Katiyar <[email protected]> wrote:
>>
>>> Fix missing iput for root inode in case of all failed mount paths.
>>> Fixes bug#26752
>>>
>>> Signed-off-by: Manish Katiyar <[email protected]>
>>>
>>> ---
>>> fs/ext4/super.c | 8 +++++++-
>>> 1 files changed, 7 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> index cb10a06..9570fcc 100644
>>> --- a/fs/ext4/super.c
>>> +++ b/fs/ext4/super.c
>>> @@ -3587,6 +3587,7 @@ no_journal:
>>> if (err) {
>>> ext4_msg(sb, KERN_ERR, "failed to initialize system "
>>> "zone (%d)", err);
>>> + iput(root);
>>> goto failed_mount4;
>>> }
>>>
>>> @@ -3595,12 +3596,15 @@ no_journal:
>>> if (err) {
>>> ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)",
>>> err);
>>> + iput(root);
>>> goto failed_mount4;
>>> }
>>>
>>> err = ext4_register_li_request(sb, first_not_zeroed);
>>> - if (err)
>>> + if (err) {
>>> + iput(root);
>>> goto failed_mount4;
>>> + }
>>>
>>> sbi->s_kobj.kset = ext4_kset;
>>> init_completion(&sbi->s_kobj_unregister);
>>> @@ -3609,6 +3613,7 @@ no_journal:
>>> if (err) {
>>> ext4_mb_release(sb);
>>> ext4_ext_release(sb);
>>> + iput(root);
>>> goto failed_mount4;
>>> };
>>>
>>> @@ -3648,6 +3653,7 @@ cantfind_ext4:
>>> goto failed_mount;
>>>
>>> failed_mount4:
>>> + sb->s_root = NULL;
>>> ext4_msg(sb, KERN_ERR, "mount failed");
>>> destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq);
>>> failed_mount_wq:
>>> --
>>> 1.7.1
>>>
>>>
>>> --
>>> Thanks -
>>> Manish
>>> ==================================
>>> [$\*.^ -- I miss being one of them
>>> ==================================
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> --
> Thanks -
> Manish
> ==================================
> [$\*.^ -- I miss being one of them
> ==================================
Cheers, Andreas
On Wed, Jul 6, 2011 at 3:54 PM, Andreas Dilger <[email protected]> wrote:
> On 2011-01-16, at 11:05 AM, Manish Katiyar wrote:
>> On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <[email protected]> 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