Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 9 Apr 2021 12:43:59 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     Jan Kara <jack@suse.cz>
Cc:     Zhang Yi <yi.zhang@huawei.com>, linux-ext4@vger.kernel.org,
        adilger.kernel@dilger.ca
Subject: Re: [PATCH] ext4: do not set SB_ACTIVE in ext4_orphan_cleanup()
Message-ID: <YHCETyV70FNVO5u6@mit.edu>
References: <20210331033138.918975-1-yi.zhang@huawei.com>
 <20210331095920.GF30749@quack2.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210331095920.GF30749@quack2.suse.cz>
Precedence: bulk

On Wed, Mar 31, 2021 at 11:59:20AM +0200, Jan Kara wrote:
> On Wed 31-03-21 11:31:38, Zhang Yi wrote:
> > When CONFIG_QUOTA is enabled, if we failed to mount the filesystem due
> > to some error happens behind ext4_orphan_cleanup(), it will end up
> > triggering a after free issue of super_block. The problem is that
> > ext4_orphan_cleanup() will set SB_ACTIVE flag if CONFIG_QUOTA is
> > enabled, after we cleanup the truncated inodes, the last iput() will put
> > them into the lru list, and these inodes' pages may probably dirty and
> > will be write back by the writeback thread, so it could be raced by
> > freeing super_block in the error path of mount_bdev().
> > 
> > After check the setting of SB_ACTIVE flag in ext4_orphan_cleanup(), it
> > was used to ensure updating the quota file properly, but evict inode and
> > trash data immediately in the last iput does not affect the quotafile,
> > so setting the SB_ACTIVE flag seems not required[1]. Fix this issue by
> > just remove the SB_ACTIVE setting.
> 
> Thanks for the patch. Let me rephrase the changelog a little:
> 
> When CONFIG_QUOTA is enabled and if we later fail to finish mounting the
> filesystem due to some error after ext4_orphan_cleanup(), we may hit use
> after free issues. The problem is that ext4_orphan_cleanup() sets SB_ACTIVE
> flag and so inodes processed during the orphan cleanup are put to the
> superblock's LRU list instead of being immediately destroyed. However the
> path handling error recovery after failed ->fill_super() call does not
> destroy inodes attached to the superblock and so they are left active in
> memory while the superblock is freed.
> 
> Originally, SB_ACTIVE setting was added so that updated quota information
> is not destroyed when we drop quota inode references after orphan cleanup.
> However VFS does not purge dirty inode pages without SB_ACTIVE flag for many
> years already. So just remove the hack with setting SB_ACTIVE flag from
> ext4_orphan_cleanup().
> 
> Also feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza

Applied, thanks.

					- Ted