Re: [PATCH 3/6] configfs: Treat attribute structures as const internally

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] configfs_create_file() and configfs_create_bin_file() use I_MUTEX_NORMAL instead of I_MUTEX_PARENT when locking the parent directory.
- [High] Incorrect subclass used when re-acquiring parent directory lock during symlink creation.
--

--- Patch [3]: [PATCH 3/6] configfs: Treat attribute structures as const internally ---
commit 02ad2fb78e37ee44180095f15441af685e40f465
Author: Thomas Weißschuh <linux@weissschuh.net>

configfs: Treat attribute structures as const internally

The configfs core never modifies the attribute structures defined in
driver core. This commit reflects this by updating the types used
internally in the configfs core.

> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> index a48cece775a39..6460b000c5938 100644
> --- a/fs/configfs/file.c
> +++ b/fs/configfs/file.c

[ ... ]

This is a pre-existing issue, but looking at configfs_create_file() in
fs/configfs/file.c:

int configfs_create_file(struct config_item * item, const struct configfs_attribute * attr)
{
	struct dentry *dir = item->ci_dentry;
	struct configfs_dirent *parent_sd = dir->d_fsdata;
	umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
	int error = 0;

	inode_lock_nested(d_inode(dir), I_MUTEX_NORMAL);
	error = configfs_make_dirent(parent_sd, NULL, (void *) attr, mode,
				     CONFIGFS_ITEM_ATTR, parent_sd->s_frag);
	inode_unlock(d_inode(dir));

	return error;
}

[Severity: High]
This isn't a bug introduced by this patch, but should this use I_MUTEX_PARENT
instead of I_MUTEX_NORMAL when locking the parent directory?

Using the wrong lockdep subclass (I_MUTEX_NORMAL instead of I_MUTEX_PARENT)
for a parent directory violates core VFS locking rules.

This defeats lockdep's ability to detect parent-child ordering violations and
can hide real deadlocks until production.


This exact pattern also occurs in configfs_create_bin_file() in the same file:

int configfs_create_bin_file(struct config_item *item,
		const struct configfs_bin_attribute *bin_attr)
{
	struct dentry *dir = item->ci_dentry;
	struct configfs_dirent *parent_sd = dir->d_fsdata;
	umode_t mode = (bin_attr->cb_attr.ca_mode & S_IALLUGO) | S_IFREG;
	int error = 0;

	inode_lock_nested(dir->d_inode, I_MUTEX_NORMAL);
	error = configfs_make_dirent(parent_sd, NULL, (void *) bin_attr, mode,
				     CONFIGFS_ITEM_BIN_ATTR, parent_sd->s_frag);
	inode_unlock(dir->d_inode);

	return error;
}

[Severity: High]
This is a pre-existing issue, but should this also be updated to use
I_MUTEX_PARENT for the same reasons?


Additionally, looking at configfs_symlink() in fs/configfs/symlink.c:

	inode_unlock(dir);
	ret = get_target(symname, &target_item, dentry->d_sb);
	inode_lock(dir);
	if (ret)
		goto out_put;

[Severity: High]
This is a pre-existing issue, but when re-acquiring the parent directory
lock, should this use inode_lock_nested(dir, I_MUTEX_PARENT)?

Calling inode_lock(dir) re-acquires the lock using the default I_MUTEX_NORMAL
lockdep subclass.

As with the file creation paths, using the wrong subclass for a parent
directory defeats lockdep's checks and hides real deadlocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-configfs-const-base-v1-0-c545a4053cb5@weissschuh.net?part=3