Re: [PATCH v2] configfs: fix slab-UAF Write in config_item_put

[Breno Leitao <leitao@debian.org>]

On Fri, Oct 23, 2026 at 05:29:06PM +0100, Drif Abdelmalek Mohamed Said wrote:
> syzbot reported:

Please drop this full log, given we have a link to the syzbot report.

> This is a race between a thread creating a symlink and another one
> doing rmdir on a parent (or ancestor) directory of symlink creation dir.
> 
> configfs_symlink() takes a reference on parent_item, the directory the
> link is created in, and then drops the directory inode lock around
> get_target(). While the lock is dropped, a concurrent rmdir can remove
> the group that parent_item belongs to.
> 
> If parent_item is a default group embedded in a larger object, such as
> nvmet_port.subsys_group, the reference on the embedded item does not
> keep the containing object alive. rmdir frees the object, and the final
> config_item_put(parent_item) in configfs_symlink() then decrements a
> kref inside freed memory.
> 
> Fix this by not holding parent_item across the unlocked window. Drop
> the reference before inode_unlock() and look it up again from
> dentry->d_parent after inode_lock(). configfs_get_config_item() returns
> NULL if the parent dentry was unhashed in the meantime, in which case
> release the target references and return -ENOENT.
> 
> Re-looking up parent_item is not sufficient on its own.
> configfs_get_config_item() only fails if the parent dentry has been
> unhashed, and the rmdir path in another thread may skip that: if
> simple_rmdir() failed, the earlier d_drop() was skipped, so the dentry
> can stay hashed even though the directory has already been detached.
> 
> Such a directory was created as a default group (CONFIGFS_USET_DIR), so
> configfs_rmdir() takes the CONFIGFS_USET_DIR branch and calls
> configfs_detach_group(). detach_groups() then marks each child inode
> S_DEAD (d_inode(child)->i_flags |= S_DEAD). Check IS_DEADDIR(dir)
> after retaking the inode lock and before taking a reference on the
> parent, and return -ENOENT. This also rejects a detached directory
> whose dentry is still hashed.

This is LLM-heavy and hard to follow, would you be able to make it a bit
more human?

> ---
> v2: Added check for dead dir, this would be helpful when configfs_remove_dir skips
> unhashing the dentry.
> v1: https://lore.kernel.org/all/20261017123748.4430-1-drifabdelmalekmohamedsaid@gmail.com/T/
>  fs/configfs/symlink.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 89178f5d371a..7d746a10c369 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -196,11 +196,26 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  	 *
>  	 *  AV, a thoroughly annoyed bastard.
>  	 */
> +
> +	/* Put the reference back, retake and check after relocking*/

Nit space before */

> +	config_item_put(parent_item);
> +	parent_item = NULL;

Why do you need to set it to NULL here?

>  	inode_unlock(dir);
>  	ret = get_target(symname, &target_item, &target_sd, dentry->d_sb);
>  	inode_lock(dir);
>  	if (ret)
> -		goto out_put;
> +		goto out_put; /* config_item_put on NULL, no problem */

This comment is not necessary.

Thanks for the patch,
--breno