Linux EXT4 FS development
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Artem Blagodarenko" <artem.blagodarenko@gmail.com>,
	linux-ext4@vger.kernel.org
Cc: tytso@mit.edu
Subject: Re: [PATCH v13 11/11] ext4: Add EXT4_IOC_SET_LUFID ioctl for setting LUFID on directory entries
Date: Thu, 29 Oct 2026 12:58:03 +0000	[thread overview]
Message-ID: <sashiko-outbox-189337@kernel.org> (raw)
In-Reply-To: <20261029122413.230409-12-ablagodarenko@thelustrecollective.com>

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] ext4: buffer overflow in ext4_insert_dentry_data() with large LUFID entries
- [High] ext4: missing dquot_initialize() in ext4_dirdata_set_lufid()
- [High] ext4: silent LUFID loss and inode orphan risk in ext4_dirdata_set_lufid()
- [High] ext4: ext4_dirdata_set_lufid() deletes dirent before strict UTF-8 check
- [High] ext4: directory entry corruption in ext4_rename() whiteout rollback
- [Medium] ext4: ext4_dirdata_set_lufid() changes filename case on encrypted casefold dirs
- [Medium] ext4: inconsistent dirent length and offset calculation in dx_get_dx_info()
- [Medium] ext4: missing IS_DIRSYNC() check in ext4_dirdata_set_lufid()
- [Medium] ext4: potential i_rwsem deadlock on subdir in ext4_dirdata_set_lufid()
--

commit cdb77af430b2349668752f74c28aae8c30a2dce5
Author: Artem Blagodarenko <artem.blagodarenko@gmail.com>

ext4: Add EXT4_IOC_SET_LUFID ioctl for setting LUFID on directory entries

This commit adds the EXT4_IOC_SET_LUFID ioctl and ext4_dirdata_set_lufid() to
set LUFID dirdata on an existing directory entry by looking up the target
entry, deleting it, and re-creating it with the LUFID payload attached.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 5a118320eef9b..4ab1f14442b09 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -543,11 +543,13 @@ static struct dx_root_info *dx_get_dx_info(struct inode *dir, void *de_buf)
>  {
>  	unsigned int blocksize = dir->i_sb->s_blocksize;
>  	void *base = de_buf;
> +	unsigned int rlen;
>  
>  	/* '.' and '..' never carry the casefold+fscrypt hash, so pass NULL
>  	 * for dir regardless of the directory's flags */
> -	if (ext4_rec_len_from_disk(((struct ext4_dir_entry_2 *)de_buf)->rec_len,
> -				   blocksize) < EXT4_BASE_DIR_LEN)
> +	rlen = ext4_rec_len_from_disk(((struct ext4_dir_entry_2 *)de_buf)->rec_len,
> +				      blocksize);
> +	if (rlen < EXT4_BASE_DIR_LEN || rlen > blocksize)
>  		return ERR_PTR(-EFSCORRUPTED);
>  	de_buf += ext4_dir_entry_len(de_buf, blocksize, NULL);
>  
> @@ -555,6 +557,11 @@ static struct dx_root_info *dx_get_dx_info(struct inode *dir, void *de_buf)
>  	if (de_buf < base || (char *)de_buf - (char *)base +
>  	    EXT4_BASE_DIR_LEN > blocksize)
>  		return ERR_PTR(-EFSCORRUPTED);
> +	rlen = ext4_rec_len_from_disk(((struct ext4_dir_entry_2 *)de_buf)->rec_len,
> +				      blocksize);
> +	if (rlen < EXT4_BASE_DIR_LEN ||
> +	    (char *)de_buf - (char *)base + rlen > blocksize)
> +		return ERR_PTR(-EFSCORRUPTED);
>  	de_buf += ext4_dir_entry_len(de_buf, blocksize, NULL);

[Severity: Medium]
In dx_get_dx_info(), rlen (from de->rec_len) is validated for both "." and
"..", but de_buf is still advanced by ext4_dir_entry_len(de_buf, blocksize,
NULL) without checking that ext4_dir_entry_len() <= rlen.

Meanwhile, get_dx_countlimit() and make_indexed_dir() locate ".." by stepping
over "." using dot_de->rec_len (rlen), and both ext4_check_dir_entry() and
ext4_check_dx_root() permit dot_de->rec_len > ext4_dir_entry_len(dot_de).

If "." has slack space (or a corrupted name_len where ext4_dir_entry_len() >
rlen), in make_indexed_dir() after bh2 is allocated, EXT4_INODE_INDEX is set
on dir, and entries after ".." are cleared from block 0, dx_get_dx_info() will
step into the slack space of "." instead of dotdot_de at offset rlen. Could
this either return -EFSCORRUPTED (leaving EXT4_INODE_INDEX set on a wiped root
block) or return a dx_root_info pointer that overlaps dotdot_de and corrupts
the root block?

Also, while this commit guards ext4_dirent_get_data_len() with
ext4_has_feature_dirdata() in ext4_htree_store_dirent(), ext4_dir_entry_len()
in fs/ext4/ext4.h still calls ext4_dirent_get_data_len() unconditionally (and
receives dir == NULL here and in get_dx_countlimit()). Should
ext4_dir_entry_len() check ext4_has_feature_dirdata() as well, and should
dx_get_dx_info() verify ext4_dir_entry_len() <= rlen (and advance past "." by
rlen)?

[ ... ]

> @@ -2374,9 +2392,9 @@ int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
>  	return 0;
>  }
>  
> -void ext4_insert_dentry_data(struct inode *dir, struct inode *inode,
> -			     struct ext4_dir_entry_2 *de, int buf_size,
> -			     struct ext4_filename *fname, void *data)
> +int ext4_insert_dentry_data(struct inode *dir, struct inode *inode,
> +			    struct ext4_dir_entry_2 *de, int buf_size,
> +			    struct ext4_filename *fname, void *data)
>  {
>  	int nlen, rlen;
>  
> @@ -2394,7 +2412,7 @@ void ext4_insert_dentry_data(struct inode *dir, struct inode *inode,
>  	ext4_set_de_type(inode->i_sb, de, inode->i_mode);
>  	de->name_len = fname_len(fname);
>  	memcpy(de->name, fname_name(fname), fname_len(fname));
> -	ext4_dirdata_set(de, dir, data, fname);
> +	return ext4_dirdata_set(de, dir, data, fname);
>  }
>  
>  /*
> @@ -2446,7 +2464,11 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname,
>  	}
>  
>  	/* By now the buffer is marked for journaling */
> -	ext4_insert_dentry_data(dir, inode, de, blocksize, fname, dfid);
> +	err = ext4_insert_dentry_data(dir, inode, de, blocksize, fname, dfid);
> +	if (err) {
> +		ext4_std_error(dir->i_sb, err);
> +		return err;
> +	}

[Severity: Critical]
Can this overflow bh->b_data or corrupt the directory block in
ext4_insert_dentry_data() when splitting a 1024-byte directory block containing
large LUFID entries?

In fs/ext4/ioctl.c:ext4_ioctl_set_lufid(), esl_name_len can be up to 255 bytes
and esl_data_len can be up to 254 bytes (ddh_length = 255):

	if (lufid_args.esl_name_len == 0 || lufid_args.esl_name_len > EXT4_NAME_LEN)
		return -EINVAL;

	if (lufid_args.esl_data_len == 0 ||
	    lufid_args.esl_data_len > 255 - sizeof(edp.df_header))
		return -EINVAL;

Including EXT4_BASE_DIR_LEN (8), the 1-byte NUL separator, and an optional
9-byte CFHASH (when casefold and encryption are enabled), a single directory
entry can now require up to 520 bytes (or 528 bytes with CFHASH).

When adding an entry overflows a leaf block, make_indexed_dir() and
ext4_dx_add_entry() call do_split() and pass its returned tail entry de
(non-NULL) to add_dirent_to_buf(handle, fname, dir, inode, de, bh). Because de
is non-NULL, add_dirent_to_buf() skips ext4_find_dest_de() and calls
ext4_insert_dentry_data() directly without verifying that rlen - nlen in de can
hold the new entry:

	/* Ensure that neither split block is over half full */
	size = 0;
	move = 0;
	for (i = count-1; i >= 0; i--) {
		/* is more than half of this entry in 2nd half of the block? */
		if (size + map[i].size/2 > blocksize/2)
			break;
		size += map[i].size;
		move++;
	}

On a 1024-byte blocksize filesystem (blocksize / 2 = 512, usable space = 1012
bytes with metadata_csum), do_split() relies on the assumption that a directory
entry is at most 264 bytes. With entries up to 520-528 bytes:

1. If a leaf block holds a single entry >= 496 bytes (count == 1) and a second
~520-byte entry is added, map[0].size / 2 <= 512 gives move = 1 and split =
count - move = 0 (with fallback split = count / 2 = 0), triggering
WARN_ON_ONCE(split == 0), ext4_error_inode_block(), and -EFSCORRUPTED.

2. If a leaf block contains 3 entries of sizes 240, 520 (with LUFID), and 252
bytes (total 1012), do_split() moves both the 252-byte and 520-byte entries
into data2 (since 252 + 520/2 = 512 <= 512), occupying 772 bytes in data2 and
leaving only rlen - nlen = 240 bytes free at offset 772 (only 252 bytes before
the end of the 1024-byte bh2->b_data buffer). When the new entry hashes to
data2 with fname_len(fname) == 255, ext4_insert_dentry_data() places de1 at
offset 772 and calls memcpy(de->name, fname_name(fname), 255) at offset 780
without checking rlen - nlen, overwriting ext4_dir_entry_tail and writing 11
bytes past the end of bh2->b_data.

3. Even when rlen - nlen >= 8 + fname_len(fname) so memcpy(de->name, ...) stays
within bh->b_data, if rlen - nlen is smaller than the full entry size with
LUFID/CFHASH (for example, 496 bytes free vs 520 bytes needed),
ext4_insert_dentry_data() has already mutated the journaled buffer in-place
(shrinking de->rec_len and writing de1->inode, de1->file_type, de1->name_len,
and de1->name) before ext4_dirdata_set() returns -EIO and add_dirent_to_buf()
calls ext4_std_error(dir->i_sb, err). That leaves a half-written active dirent
in the block and causes the rollback ext4_add_entry() in
ext4_dirdata_set_lufid() to fail with -EEXIST.

[ ... ]

> @@ -4581,6 +4609,21 @@ static int ext4_rename(struct mnt_idmap *idmap, struct inode *old_dir,
>  	ext4_lufid_snap_release(&old_snap);
>  	if (whiteout) {
>  		if (retval) {
> +			/* Known limitation: if the old entry carried both LUFID
> +			 * and CFHASH, ext4_setent_compact_exts() slid CFHASH
> +			 * into the LUFID slot above, and old_snap was released
> +			 * before this point, so resetent can only restore the
> +			 * LUFID flag — not the original byte layout.  This
> +			 * leaves the entry with CFHASH data at the LUFID offset
> +			 * and garbage at the CFHASH offset until the next
> +			 * SET_LUFID ioctl re-writes it.  The scenario requires
> +			 * RENAME_WHITEOUT on a casefolded+encrypted dirdata
> +			 * directory whose entry has a LUFID, followed by a
> +			 * journal IO error in the single ext4_mark_inode_dirty()
> +			 * call that follows — an extremely unlikely combination
> +			 * in practice.  (OOM during the LUFID wrapper allocation
> +			 * cannot trigger this path because that allocation is
> +			 * hoisted before ext4_setent() modifies the entry.) */
>  			ext4_resetent(handle, &old,
>  				      old.inode->i_ino, old_file_type);
>  			drop_nlink(whiteout);

[Severity: High]
Does this whiteout rollback path in ext4_rename() permanently corrupt old.de
even without a journal IO error, and can SET_LUFID actually recover it?

After the whiteout block in ext4_rename() calls ext4_setent() (which calls
ext4_setent_compact_exts() and slides the 9-byte CFHASH into the LUFID slot at
loff) and ext4_mark_inode_dirty(handle, whiteout), execution continues to:

	if (!new.bh) {
		new.dentry->d_fsdata = edp;
		retval = ext4_add_entry(handle, new.dentry, old.inode);
		new.dentry->d_fsdata = NULL;
		if (retval)
			goto end_rename;

If ext4_add_entry() fails under normal conditions such as -ENOSPC or -EDQUOT
when allocating a block for the destination directory, it jumps to end_rename.
At end_rename, ext4_lufid_snap_release(&old_snap) frees old_snap before the
if (whiteout) block runs, and ext4_resetent() calls ext4_setent() with dfid ==
NULL, which restores the EXT4_DIRENT_LUFID flag in old.de->file_type while the
9-byte CFHASH (with dh_header.ddh_length == 9) is still sitting at the LUFID
offset loff.

Once that happens, any subsequent lookup (including ext4_find_entry() in
ext4_dirdata_set_lufid()) calls ext4_match() -> ext4_dirdata_get(), which skips
9 bytes for LUFID and reads stale bytes at loff + 9 as the CFHASH. Doesn't the
hash check in ext4_match() then fail on every lookup, making the entry
permanently unreachable to lookup, unlink, or EXT4_IOC_SET_LUFID while leaving
the directory non-empty? Why not pass old_snap.fid to ext4_resetent() and
release old_snap afterwards?

> @@ -4793,6 +4836,266 @@ static int ext4_rename2(struct mnt_idmap *idmap,
>  	return ext4_rename(idmap, old_dir, old_dentry, new_dir, new_dentry, flags);
>  }
>  
> +/*
> + * ext4_dirdata_set_lufid() - Set LUFID data on an existing directory entry
> + * @dir:        parent directory inode
> + * @filename:   name of the file in the directory
> + * @namelen:    length of filename
> + * @edp:        pointer to initialized dentry param with LUFID data
> + *
> + * This function finds an existing directory entry, deletes it, and re-creates it
> + * with LUFID data attached. Used by the EXT4_IOC_SET_LUFID ioctl.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int ext4_dirdata_set_lufid(struct mnt_idmap *idmap, struct inode *dir,
> +			    const char *filename, int namelen,
> +			    struct ext4_dentry_param *edp)
> +{

[ ... ]

> +	d_name.name = filename;
> +	d_name.len = namelen;
> +
> +	err = ext4_fname_setup_filename(dir, &d_name, 0, &fname);
> +	if (err)
> +		goto out_free;
> +
> +	/* Lock dir with the VFS parent-mutation subclass.  The lookup and any
> +	 * mutation must be inside this lock to prevent TOCTOU races. */
> +	inode_lock_nested(dir, I_MUTEX_PARENT);

[Severity: High]
Should ext4_dirdata_set_lufid() call dquot_initialize(dir) before starting the
journal transaction?

Opening a directory via ext4_dir_open() and looking up an inode via ext4_iget()
do not initialize quotas on dir, leaving dir->i_dquot[] entries NULL if no
prior VFS mutation has initialized them. Every other directory-mutating
operation in fs/ext4/namei.c calls dquot_initialize(dir) before
ext4_journal_start().

When ext4_dirdata_set_lufid() deletes the entry and re-adds it with LUFID data
via ext4_add_entry(), the larger entry may not fit in existing inline data or
directory blocks, causing ext4_add_entry() to allocate new directory blocks
(via ext4_convert_inline_data_nolock(), make_indexed_dir(), do_split(), or
ext4_append()). Without dquot_initialize(dir), dquot_alloc_block(dir, ...)
skips quota enforcement and does not charge the allocated blocks while
dir->i_blocks is incremented, which allows bypassing quota limits and causes
quota counter underflow (and warnings in dquot_free_space_nodirty() or
ext4_expand_extra_isize_ea()) when quotas on dir are later initialized and the
blocks are freed.

[ ... ]

> +	/* Lock the target inode BEFORE starting the journal (dir-then-child,
> +	 * VFS-locks-before-journal ordering).  Use I_MUTEX_CHILD for directory
> +	 * inodes (consistent with VFS rename/rmdir) and I_MUTEX_NONDIR2 for
> +	 * all others; mixing subclasses on the same inode triggers lockdep
> +	 * cycles with concurrent rename. */
> +	if (inode != dir) {
> +		if (S_ISDIR(inode->i_mode))
> +			inode_lock_nested(inode, I_MUTEX_CHILD);
> +		else
> +			inode_lock_nested(inode, I_MUTEX_NONDIR2);
> +		child_locked = true;
> +	}

[Severity: Medium]
Can locking a directory inode with inode_lock_nested(inode, I_MUTEX_CHILD) here
cause an AB-BA deadlock on i_rwsem if a corrupted on-disk directory entry in
dir references an ancestor directory?

In normal VFS path walks, ext4_lookup() -> d_splice_alias() checks
d_ancestor() under rename_lock and rejects directory loops with -ELOOP before
callers like vfs_rmdir() or vfs_rename() lock the child directory inode. Here,
ext4_dirdata_set_lufid() reads ino directly from the on-disk de->inode via
ext4_find_entry() and ext4_iget() while holding dir->i_rwsem, bypassing dcache
ancestor checks and sb->s_vfs_rename_mutex. If de->inode points to an ancestor
directory, locking inode->i_rwsem while holding dir->i_rwsem inverts the
ancestor-before-descendant locking order against concurrent path walks, rmdir,
or rename.

Since deleting and re-adding the entry within the same parent directory only
mutates dir's blocks and does not modify a child directory inode, its i_nlink,
or its ".." entry (similar to same-directory rename in vfs_rename() where
lock_old_subdir is false when new_dir == old_dir), is locking a subdirectory
inode needed here at all?

> +
> +	handle = ext4_journal_start(dir, EXT4_HT_DIR,
> +				    3 * EXT4_DATA_TRANS_BLOCKS(sb) +
> +				    2 * EXT4_INDEX_EXTRA_TRANS_BLOCKS);
> +	if (IS_ERR(handle)) {
> +		err = PTR_ERR(handle);
> +		handle = NULL;
> +		goto out_unlock;
> +	}

[Severity: Medium]
Should ext4_dirdata_set_lufid() check IS_DIRSYNC(dir) and call
ext4_handle_sync(handle) after starting the transaction, as the other
directory-mutating operations in fs/ext4/namei.c do? Without it, neither
ext4_delete_entry() nor ext4_add_entry() marks the handle synchronous, so
ext4_journal_stop() will return without committing the transaction on a
synchronous directory.

[ ... ]

> +	/* Choose the name to use for re-adding the entry after deletion.
> +	 *
> +	 * For non-encrypted casefolded directories ext4_find_entry() matches
> +	 * case-insensitively, so de->name may differ in case from the
> +	 * caller-supplied filename.  Re-adding with the caller's case would
> +	 * silently rename the entry, so preserve the on-disk bytes.
> +	 *
> +	 * For encrypted directories (with or without casefold) de->name holds
> +	 * the raw ciphertext.  ext4_fname_setup_filename() deterministically
> +	 * re-encrypts d_name (the plaintext) to the identical ciphertext, so
> +	 * using d_name for the re-add is correct.  Passing the raw ciphertext
> +	 * bytes would cause double-encryption and make the entry unreachable. */
> +	if (IS_CASEFOLDED(dir) && !IS_ENCRYPTED(dir)) {
> +		memcpy(ondisk_name, de->name, de->name_len);
> +		real_name.name = ondisk_name;
> +		real_name.len = de->name_len;
> +		add_name = &real_name;
> +	} else {
> +		add_name = &d_name;
> +	}

[Severity: Medium]
When both IS_CASEFOLDED(dir) and IS_ENCRYPTED(dir) are true, doesn't using
add_name = &d_name overwrite the on-disk filename's case with the caller's
case?

In ext4_fname_setup_filename(), fscrypt_setup_filename() encrypts the exact
un-casefolded plaintext name (for example, "FooBar" -> Enc("FooBar")), while
ext4_fname_setup_ci_filename() computes the hash of the casefolded name
("foobar") for CFHASH. When ext4_find_entry() is called with d_name = "foobar",
ext4_match() matches the CFHASH and generic_ci_match() decrypts de->name to
"FooBar" and matches it case-insensitively.

Because fscrypt encryption is deterministic on the exact plaintext bytes rather
than the casefolded bytes, re-adding the entry with add_name = &d_name will
encrypt "foobar" into Enc("foobar") != Enc("FooBar"), silently changing the
on-disk filename's casing (and potentially its byte length for multi-byte UTF-8
casefold mappings) and leaving any cached positive dentry in the VFS dcache out
of sync with the on-disk entry.

> +
> +	/* Snapshot the old LUFID before deleting so we can restore it if the
> +	 * re-add fails.  Without this the rollback path re-adds the entry
> +	 * without any LUFID, silently orphaning the old FID-to-path mapping. */
> +	err = ext4_lufid_snapshot(de, sb->s_blocksize, &old_snap);
> +	if (err)
> +		goto out_brelse;
> +
> +	err = ext4_delete_entry(handle, dir, de, bh);
> +	if (err)
> +		goto out_brelse;
> +
> +	brelse(bh);
> +	bh = NULL;
> +
> +	/* Re-add with LUFID via dentry->d_fsdata.  ext4_add_entry() resolves
> +	 * dfid from d_fsdata into fname.dfid and passes it through to
> +	 * add_dirent_to_buf() without touching the shared i_dirdata field,
> +	 * eliminating the race with concurrent link() calls. */
> +	{
> +		struct dentry parent_dentry = { .d_inode = dir };
> +		struct dentry new_dentry = {
> +			.d_name = *add_name,
> +			.d_parent = &parent_dentry,
> +			.d_inode = inode,
> +			.d_fsdata = edp,
> +		};
> +		err = ext4_add_entry(handle, &new_dentry, inode);
> +	}

[Severity: High]
What happens on a casefolded directory with strict encoding enabled
(SB_ENC_STRICT_MODE_FL) if an existing entry has invalid UTF-8 bytes in its
name (for instance, created before strict encoding was enabled)?

In ext4_dirdata_set_lufid(), ext4_fname_setup_filename() ->
ext4_fname_setup_ci_filename() ignores utf8_casefold() failure by setting
cf_name->name = NULL and returning 0, and ext4_find_entry() ->
generic_ci_match() matches the existing entry via its exact byte-for-byte
memcmp() fast path without checking sb_has_strict_encoding().

However, after ext4_delete_entry() deletes the entry from the directory,
ext4_add_entry() -> __ext4_add_entry() checks:

	if (!generic_ci_validate_strict_name(dir, d_name))
		return -EINVAL;

Both the primary ext4_add_entry(handle, &new_dentry, inode) and the rollback
ext4_add_entry(handle, &orig_dentry, inode) will then deterministically fail
with -EINVAL, triggering EXT4_ERROR_INODE() and leaving the directory entry
deleted and the target inode orphaned. Should generic_ci_validate_strict_name()
be checked before calling ext4_delete_entry()?

> +
> +	if (err) {
> +		/* Delete succeeded but re-add failed; try to restore so the
> +		 * inode is not left without a directory entry.  Restore the
> +		 * original LUFID too: without it the entry is re-added clean
> +		 * and the old FID-to-path mapping is silently lost. */
> +		struct dentry parent_dentry = { .d_inode = dir };
> +		struct dentry orig_dentry = {
> +			.d_name = *add_name,
> +			.d_parent = &parent_dentry,
> +			.d_inode = inode,
> +		};
> +		int rollback_err;
> +
> +		if (old_snap.fid) {
> +			unsigned int fid_size = old_snap.fid->df_header.ddh_length;
> +			struct ext4_dentry_param *old_edp =
> +				kmalloc(sizeof(*old_edp) + fid_size, GFP_NOFS);
> +
> +			if (old_edp) {
> +				old_edp->edp_magic = EXT4_LUFID_MAGIC;
> +				memcpy(old_edp->edp_dfid, old_snap.fid, fid_size);
> +				orig_dentry.d_fsdata = old_edp;
> +			}
> +		}
> +
> +		rollback_err = ext4_add_entry(handle, &orig_dentry, inode);
> +		kfree(orig_dentry.d_fsdata);
> +
> +		if (rollback_err)
> +			EXT4_ERROR_INODE(dir,
> +				"Failed to set LUFID on '%.*s' (err=%d) and failed to restore the original directory entry (err=%d); inode %llu may be orphaned",
> +				namelen, filename, err, rollback_err,
> +				(unsigned long long)inode->i_ino);
> +		goto out_journal;
> +	}

[Severity: High]
Why is old_edp allocated with kmalloc(..., GFP_NOFS) after ext4_delete_entry()
has already removed the entry, rather than being pre-allocated before the
deletion like edp in ext4_rename()?

If the primary ext4_add_entry() fails (for example with -ENOSPC, -EDQUOT, or
-ENOMEM) and the GFP_NOFS allocation of old_edp returns NULL,
orig_dentry.d_fsdata remains NULL and the rollback ext4_add_entry() silently
re-adds the entry without its original LUFID.

Also, ext4_dirdata_set_lufid() calls ext4_fname_setup_filename(dir, &d_name, 0,
&fname) at the start of the function, but fname is never used. Instead,
ext4_add_entry() -> __ext4_add_entry() calls ext4_fname_setup_filename() again
under GFP_NOFS during both the re-add and the rollback after the entry has
already been deleted. If the rollback ext4_add_entry() fails (for instance due
to -ENOMEM in ext4_fname_setup_filename()), ext4_journal_stop() still commits
the transaction with the directory entry deleted and without updating
inode->i_nlink or adding inode to the orphan list. Could old_edp and the
filename buffers be prepared before ext4_delete_entry() so rollback does not
depend on post-deletion allocations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029122413.230409-1-ablagodarenko@thelustrecollective.com?part=11

      reply	other threads:[~2026-10-29 12:58 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-29 12:23 [PATCH v13 00/11] Data in direntry (dirdata) feature Artem Blagodarenko
2026-10-29 12:24 ` [PATCH v13 01/11] ext4: validate count against limit in ext4_dx_csum_verify/_set Artem Blagodarenko
2026-10-29 12:37   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 02/11] ext4: replace ext4_dir_entry with ext4_dir_entry_2 Artem Blagodarenko
2026-10-29 12:34   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 03/11] ext4: add ext4_dir_entry_is_tail() Artem Blagodarenko
2026-10-29 12:31   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 04/11] ext4: add dirdata format definitions and access helpers Artem Blagodarenko
2026-10-29 12:31   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 05/11] ext4: refactor dx_root to support variable dirent sizes Artem Blagodarenko
2026-10-29 12:49   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 06/11] ext4: add ext4_dir_entry_len() Artem Blagodarenko
2026-10-29 12:50   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 07/11] ext4: rename ext4_dir_rec_len() and clarify dirdata usage Artem Blagodarenko
2026-10-29 12:50   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 08/11] ext4: dirdata feature Artem Blagodarenko
2026-10-29 12:51   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 09/11] ext4: add dirdata LUFID support for directory entry rename Artem Blagodarenko
2026-10-29 12:47   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 10/11] ext4: add dirdata set/get helpers Artem Blagodarenko
2026-10-29 12:44   ` sashiko-bot
2026-10-29 12:24 ` [PATCH v13 11/11] ext4: Add EXT4_IOC_SET_LUFID ioctl for setting LUFID on directory entries Artem Blagodarenko
2026-10-29 12:58   ` sashiko-bot [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=sashiko-outbox-189337@kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=artem.blagodarenko@gmail.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox