Re: [PATCH v2 4/5] fs/resctrl: Fix deadlock for errors during mount

[Babu Moger <babu.moger@amd.com>]

Hi Tony,


On 5/15/26 14:39, Tony Luck wrote:
> From: Reinette Chatre <reinette.chatre@intel.com>
> 
> Sashiko noticed[1] a deadlock in the resctrl mount code.
> 
> rdt_get_tree() acquires rdtgroup_mutex before calling kernfs_get_tree(). If
> superblock setup fails inside kernfs_get_tree(), the VFS calls kill_sb on
> the same thread before the call returns.  rdt_kill_sb() unconditionally
> attempts to acquire rdtgroup_mutex and deadlock occurs.
> 
> Move the call to kernfs_get_tree() outside of locks.
> 
> Add resctrl_unmount() helper to keep code consistent between the
> rdt_get_tree() failure path and a normal unmount.
> 
> If kernfs_get_tree() fails and ctx->kfc.new_sb_created is set, then rdt_kill_sb()
> has already been called and no further cleanup is needed.
> 
> Add an extra hold in this error path on rdtgroup_default.kn to defend against
> other races destroying the root which is then dereferenced in kernfs_kill_sb()
> 
> Fixes: 5ff193fbde20 ("x86/intel_rdt: Add basic resctrl filesystem support")
> Co-developed-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>

This should be

Co-developed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by:   Tony Luck <tony.luck@intel.com>
Signed-off-by:   Reinette Chatre <reinette.chatre@intel.com>

Thanks
Babu

> Link: https://sashiko.dev/#/patchset/20260429184858.36423-1-tony.luck%40intel.com [1]
> ---
>   fs/resctrl/rdtgroup.c | 82 +++++++++++++++++++++++++++++--------------
>   1 file changed, 55 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 97d1a3648b9e..282a0acedea8 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2978,10 +2978,34 @@ static void resctrl_fs_teardown(void)
>   	rdtgroup_destroy_root();
>   }
>   
> +static void resctrl_unmount(void)
> +{
> +	struct rdt_resource *r;
> +
> +	cpus_read_lock();
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	rdt_disable_ctx();
> +
> +	/* Put everything back to default values. */
> +	for_each_alloc_capable_rdt_resource(r)
> +		resctrl_arch_reset_all_ctrls(r);
> +
> +	resctrl_fs_teardown();
> +	if (resctrl_arch_alloc_capable())
> +		resctrl_arch_disable_alloc();
> +	if (resctrl_arch_mon_capable())
> +		resctrl_arch_disable_mon();
> +	resctrl_mounted = false;
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +}
> +
>   static int rdt_get_tree(struct fs_context *fc)
>   {
>   	struct rdt_fs_context *ctx = rdt_fc2context(fc);
>   	unsigned long flags = RFTYPE_CTRL_BASE;
> +	struct kernfs_node *rdt_root_kn;
>   	struct rdt_l3_mon_domain *dom;
>   	struct rdt_resource *r;
>   	int ret;
> @@ -3057,10 +3081,6 @@ static int rdt_get_tree(struct fs_context *fc)
>   	if (ret)
>   		goto out_mondata;
>   
> -	ret = kernfs_get_tree(fc);
> -	if (ret < 0)
> -		goto out_psl;
> -
>   	if (resctrl_arch_alloc_capable())
>   		resctrl_arch_enable_alloc();
>   	if (resctrl_arch_mon_capable())
> @@ -3076,10 +3096,37 @@ static int rdt_get_tree(struct fs_context *fc)
>   						   RESCTRL_PICK_ANY_CPU);
>   	}
>   
> -	goto out;
> +	/*
> +	 * Ensure root kn remains accessible after mutex is unlocked so that
> +	 * kernfs_kill_sb() can run safely if called by kernfs_get_tree()'s
> +	 * failure path after creating a superblock but before taking reference
> +	 * on root kn.
> +	 */
> +	kernfs_get(rdtgroup_default.kn);
> +
> +	/*
> +	 * Make backup of the current root kn being created to be used in kernfs_put().
> +	 * The additional reference taken above will prevent the kn from being freed
> +	 * before kernfs_kill_sb() can run but rdtgroup_default.kn may be set to NULL
> +	 * via rdtgroup_destroy_root() and its backing root (rdt_root) could be overwritten
> +	 * before kernfs_put() can run.
> +	 */
> +	rdt_root_kn = rdtgroup_default.kn;
> +
> +	rdt_last_cmd_clear();
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +
> +	ret = kernfs_get_tree(fc);
> +	/*
> +	 * resctrl can only be mounted once, new superblock only expected
> +	 * to be created once.
> +	 */
> +	if (!ctx->kfc.new_sb_created)
> +		resctrl_unmount();
> +	kernfs_put(rdt_root_kn);
> +	return ret;
>   
> -out_psl:
> -	rdt_pseudo_lock_release();
>   out_mondata:
>   	if (resctrl_arch_mon_capable())
>   		kernfs_remove(kn_mondata);
> @@ -3099,7 +3146,6 @@ static int rdt_get_tree(struct fs_context *fc)
>   out_root:
>   	rdtgroup_destroy_root();
>   out:
> -	rdt_last_cmd_clear();
>   	mutex_unlock(&rdtgroup_mutex);
>   	cpus_read_unlock();
>   	return ret;
> @@ -3186,26 +3232,8 @@ static int rdt_init_fs_context(struct fs_context *fc)
>   
>   static void rdt_kill_sb(struct super_block *sb)
>   {
> -	struct rdt_resource *r;
> -
> -	cpus_read_lock();
> -	mutex_lock(&rdtgroup_mutex);
> -
> -	rdt_disable_ctx();
> -
> -	/* Put everything back to default values. */
> -	for_each_alloc_capable_rdt_resource(r)
> -		resctrl_arch_reset_all_ctrls(r);
> -
> -	resctrl_fs_teardown();
> -	if (resctrl_arch_alloc_capable())
> -		resctrl_arch_disable_alloc();
> -	if (resctrl_arch_mon_capable())
> -		resctrl_arch_disable_mon();
> -	resctrl_mounted = false;
> +	resctrl_unmount();
>   	kernfs_kill_sb(sb);
> -	mutex_unlock(&rdtgroup_mutex);
> -	cpus_read_unlock();
>   }
>   
>   static struct file_system_type rdt_fs_type = {