Re: [RFC v7 4/7] ext4: fast commit: avoid self-deadlock in inode snapshotting

Hi sashiko,

 ---- On Tue, 12 May 2026 11:46:39 +0800   <[email protected]> wrote --- 
 > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
 > - [Critical] Removing igrab() exposes a critical use-after-free vulnerability because ext4_fc_del() can miss the EXT4_STATE_FC_COMMITTING state transition and free the inode while it is actively being snapshotted.
 > --
 > 
 > commit 39c5450f50154240ff764cf3a63e011d8e973fd4
 > Author: Li Chen <[email protected]>
 > 
 > ext4: fast commit: avoid self-deadlock in inode snapshotting
 > 
 > This commit removes igrab()/iput() during fast commit snapshotting, relying
 > instead on the EXT4_STATE_FC_COMMITTING flag to keep inodes pinned while
 > their pointers are stored in the inodes[] array for snapshotting.
 > 
 > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
 > > index 273bf34031aeb..f9bb18c0b5497 100644
 > > --- a/fs/ext4/fast_commit.c
 > > +++ b/fs/ext4/fast_commit.c
 > > @@ -1195,13 +1195,12 @@ static int ext4_fc_snapshot_inodes(journal_t *journal)
 > >  
 > >      alloc_ctx = ext4_fc_lock(sb);
 > >      list_for_each_entry(iter, &sbi->s_fc_q[FC_Q_MAIN], i_fc_list) {
 > > -        inodes[i] = igrab(&iter->vfs_inode);
 > > -        if (inodes[i])
 > > -            i++;
 > > +        inodes[i++] = &iter->vfs_inode;
 > >      }
 > 
 > Is it possible for this change to cause a use-after-free of the inode?
 > 
 > If ext4_fc_del() is called during inode eviction, it waits sequentially for
 > EXT4_STATE_FC_COMMITTING and then EXT4_STATE_FC_FLUSHING_DATA. However, it
 > doesn't appear to loop back to re-check EXT4_STATE_FC_COMMITTING after
 > waiting for the flush bit.
 > 
 > Could a sequence like this occur?
 > 
 > 1. ext4_fc_perform_commit() sets EXT4_STATE_FC_FLUSHING_DATA.
 > 2. Eviction calls ext4_fc_del(), which sees EXT4_STATE_FC_COMMITTING is
 >    false, but EXT4_STATE_FC_FLUSHING_DATA is true, so it waits on the
 >    flush bit.
 > 3. ext4_fc_perform_commit() clears EXT4_STATE_FC_FLUSHING_DATA, waking up
 >    ext4_fc_del().
 > 4. Before ext4_fc_del() resumes, ext4_fc_perform_commit() sets
 >    EXT4_STATE_FC_COMMITTING and saves the unpinned inode pointer into the
 >    inodes[] array.
 > 5. ext4_fc_del() resumes, skips re-checking EXT4_STATE_FC_COMMITTING,
 >    removes the inode from the fast commit list, and returns.
 > 6. The VFS completely frees the inode.
 > 7. ext4_fc_snapshot_inodes() iterates over the inodes[] array and passes
 >    the now dangling pointer to ext4_fc_snapshot_inode().
 > 
 > Does ext4_fc_del() need to re-verify the state flags after waking up, or
 > is there another mechanism preventing the inode from being freed before
 > ext4_fc_snapshot_inode() accesses it?
 > 
 > [ ... ]
 > 
 > -- 
 > Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
 > 

Yes, I think you're right. I missed that window.

I'll make ext4_fc_del() loop back after waiting on FC_FLUSHING_DATA, so it checks FC_COMMITTING again
under s_fc_lock before removing the inode from the FC lists.

Regards,
Li​