Re: [PATCH 1/9] binfmt_misc: let a register string create an entry disabled

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Inconsistent VFS lockdep subclass usage for the root directory's i_rwsem in bm_entry_write().
--

--- Patch [1]: [PATCH 1/9] binfmt_misc: let a register string create an entry disabled ---
commit 804ba816d20f57c8b2fa3e1866d3a8ae54eeeb11
Author: Christian Brauner <[email protected]>

binfmt_misc: let a register string create an entry disabled

This commit adds a 'D' flag to binfmt_misc registration, allowing entries to
be created in a disabled state and activated later. This separates creation
from activation and keeps staged entries out of the search list until enabled.

> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 707f8a14f8a63..ca7840b01a2bb 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c

[ ... ]

> @@ -1067,9 +1110,27 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
>  	case BM_CMD_DISABLE:
>  		clear_bit(MISC_FMT_ENABLED_BIT, &e->flags);
>  		break;
> -	case BM_CMD_ENABLE:
> +	case BM_CMD_ENABLE: {
> +		struct inode *root = bm_root_inode(inode->i_sb);
> +
> +		/*
> +		 * The first enable publishes a 'D' entry into the search
> +		 * list, whole. The lock keeps that ordered against a second
> +		 * enable and against removal; a removed entry has nothing
> +		 * left to publish.
> +		 */
> +		inode_lock(root);

[Severity: Medium]
In bm_remove_entry() and bm_remove_all_entries(), the root directory lock is
acquired using inode_lock_nested(root, I_MUTEX_PARENT). Here in
bm_entry_write(), we're using inode_lock(root) which uses the default subclass
(I_MUTEX_NORMAL).

Does this inconsistent VFS lockdep subclass usage defeat lockdep's parent-child
ordering checks? Using different subclasses for the same directory's i_rwsem
might split the lockdep tracking and hide potential deadlocks from the
validator.

Should this be inode_lock_nested(root, I_MUTEX_PARENT) to match the removal
paths?

>  		set_bit(MISC_FMT_ENABLED_BIT, &e->flags);
> +		if (hlist_unhashed(&e->node) && !d_unhashed(e->dentry)) {
> +			struct binfmt_misc *misc = i_binfmt_misc(inode);
> +
> +			spin_lock(&misc->entries_lock);
> +			hlist_add_head_rcu(&e->node, &misc->entries);
> +			spin_unlock(&misc->entries_lock);
> +		}
> +		inode_unlock(root);
>  		break;
> +	}
>  	case BM_CMD_REMOVE:
>  		bm_remove_entry(e, inode->i_sb);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1