linux-ext4 - Re: [PATCH] ext4: fix warning when submitting superblock in ext4_commit

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20220518170617.vooz4ycfe73xsszx@riteshh-domain>
Date:   Wed, 18 May 2022 22:36:17 +0530
From:   Ritesh Harjani <ritesh.list@...il.com>
To:     Zhang Yi <yi.zhang@...wei.com>, Jan Kara <jack@...e.cz>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, jack@...e.cz, yukuai3@...wei.com
Subject: Re: [PATCH] ext4: fix warning when submitting superblock in
 ext4_commit_super()

On 22/05/18 10:10PM, Zhang Yi wrote:
> We have already check the io_error and uptodate flag before submitting
> the superblock buffer, and re-set the uptodate flag if it has been
> failed to write out. But it was lockless and could be raced by another
> ext4_commit_super(), and finally trigger '!uptodate' WARNING when
> marking buffer dirty. Fix it by submit buffer directly.

I agree that there could be a race with multiple processes trying to call
ext4_commit_super(). Do you have a easy reproducer for this issue?

Also do you think something like below should fix the problem too?
So if you lock the buffer from checking until marking the buffer dirty, that
should avoid the race too that you are reporting.
Thoughts?


diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6900da973ce2..3447841fe654 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6007,6 +6007,7 @@ static int ext4_commit_super(struct super_block *sb)

        ext4_update_super(sb);

+       lock_buffer(sbh);
        if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) {
                /*
                 * Oh, dear.  A previous attempt to write the
@@ -6023,6 +6024,7 @@ static int ext4_commit_super(struct super_block *sb)
        }
        BUFFER_TRACE(sbh, "marking dirty");
        mark_buffer_dirty(sbh);
+       unlock_buffer(sbh);
        error = __sync_dirty_buffer(sbh,
                REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0));
        if (buffer_write_io_error(sbh)) {