[PATCH v3 RESEND] ext4: clear extent index structure after file delete

[PATCH v3 RESEND] ext4: clear extent index structure after file delete

[Nicolas Bretz]

The extent index structure in the top inode is not being cleared after a file
is deleted, which leaves the path to the data blocks intact. This patch clears
this extent index structure.

Extent structures are already being cleared, so this also makes the
behavior consistent between extent and extent _index_ structures.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=220342

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202507210558.sazSHcm1-lkp@intel.com/
Signed-off-by: Nicolas Bretz <bretznic@gmail.com>
---
Changes in v2:
- corrected function name. Due to my incorrect use of git, attempting to ammend only the message led to code changes being reverted, after building successfully.
Changes in v3:
- corrected sparse: restricted __le16 degrades to integer
---
 fs/ext4/extents.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index b543a46fc809..17591a99dafd 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2822,6 +2822,7 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	int depth = ext_depth(inode);
 	struct ext4_ext_path *path = NULL;
+	struct ext4_extent_idx *ix = NULL;
 	struct partial_cluster partial;
 	handle_t *handle;
 	int i = 0, err = 0;
@@ -3060,6 +3061,9 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
 		 */
 		err = ext4_ext_get_access(handle, inode, path);
 		if (err == 0) {
+			ix = EXT_FIRST_INDEX(path->p_hdr);
+			if (ix && le16_to_cpu(ext_inode_hdr(inode)->eh_depth) > 0)
+				ext4_idx_store_pblock(ix, 0);
 			ext_inode_hdr(inode)->eh_depth = 0;
 			ext_inode_hdr(inode)->eh_max =
 				cpu_to_le16(ext4_ext_space_root(inode, 0));
-- 
2.47.2

Re: [PATCH v3 RESEND] ext4: clear extent index structure after file delete

[Theodore Ts'o]

On Wed, Sep 03, 2025 at 04:30:27AM -0700, Nicolas Bretz wrote:
> The extent index structure in the top inode is not being cleared after a file
> is deleted, which leaves the path to the data blocks intact. This patch clears
> this extent index structure.
> 
> Extent structures are already being cleared, so this also makes the
> behavior consistent between extent and extent _index_ structures.

Actually, if we are going to make things consistent, we would be *not*
be clearing the extent leaf blocks if we are deleting the file ---
when possible.

Clearing the extent structures was never for security concerns.  The
reality is that removing the pointers to the data blocks is security
theater (e.g., like the TSA in US airports).  It makes people feel
good, but programs like photorec can be used to find the data blocks.
If they really want to securly delete a file, they should use shred or
wipe to overwrite the datablocks before deleting the file.

[1] https://www.cgsecurity.org/wiki/photoRec

The reason why we wipe the extent structures is because when
journalling is enabled, a file truncation or deletion might not fit in
a single journal transaction, and might need to span two transactions.
For that reason, we put the inode on the orphan list, and then if the
operation doesn't fit in a single transaction, we need to keep the
file system in a consistent state at each transaction boundary.  So
that's why we zero out the extent structures as we go; so if we need
to pause the truncation so we can do a journal commit, the data block
pointers to the blocks that have been released are properly zeroed.

Now, if we know that all of the blocks in an extent leaf block can be
released in the current transaction, we could omit zeroing the leaf
block --- so long as we can drop the pointer to the leaf block in the
parent index block.  This also has the benefit that if we don't need
to modify the extent leaf block, we save two 4k writes to the disk ---
one in the journal and one in the extent leaf block, which would
improve the performance of an "rm -rf" workload.

The reason why we haven't done this is that the benefits aren't that
big, and so we haven't gotten around toit.  But if you are interested
in looking into it, if we can keep the code complexity down and avoid
impacting the maintainability of the code base feel free to take a
look at it.

					- Ted

P.S.  A related project would be adding support for the "secure
deletion" flag (see the chattr man page), which is currently
unimplemented.  The tricky bit is (a) we can't zero the blocks until
the transaction releasing the blockshas been commited, and (b) we need
to avoid a race where a block being zero gets reallocated since we
don't want to zero out data belonging to a newly realocated data block
that has been associated with an in-use inode.  The marginal utility
is a bit small, since userspace tools like shred and wipe already
exist, which is why no one has actually implemented to date.  But if
you're looking for an fun/interesting project, it's a possibility.