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

[Luck, Tony]

On Wed, May 13, 2026 at 03:19:40PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 5/13/26 12:51 PM, Luck, Tony wrote:
> ...
> > Are we out of the woods yet? Applying these suggestions I now have:
> > 
> > 	/* Ensure root kn remains accessible after mutex is unlocked */
> > 	kernfs_get(rdtgroup_default.kn);
> > 
> > 	/*
> > 	 * Make backup of rdtgroup_default.kn just in case one of the
> > 	 * following flows (that sets rdtgroup_default.kn to NULL) run after
> > 	 * the mutex is unlocked:
> > 	 * resctrl_exit()->resctrl_fs_teardown()->rdtgroup_destroy_root()
> > 	 * kernfs_get_tree()->deactivate_locked_super()->rdt_kill_sb()->
> > 	 *	resctrl_unmount()->resctrl_fs_teardown()->rdtgroup_destroy_root()
> > 	 * These flows would not actually result in rdtgroup_default.kn
> > 	 * being removed thanks to the additional reference.
> > 	 */
> > 	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();
> > 
> > resctrl_unmount() clears resctrl_mounted, so as soon locks are released
> > a new mount attempt (maybe started a while ago, but blocked waiting for
> > the mutex) can begin. I just want to confirm that won't stomp on
> > anything left over from this failed mount that was waiting for this
> > kernfs_put() to happen.  I think it is OK, because the new mount is
> > going to allocate all new structures. But there's been enough layers
> > to this onion that I'd like to confirm.
> > 
> > 	kernfs_put(rdt_root_kn);
> > 	return ret;
> 
> I agree with your analysis. I also think it highlights a sharp corner
> that may benefit from a comment. The rdt_root_kn is intentionally a local
> variable and the comments above explain that it is needed because of some
> flows that may set rdtgroup_default.kn to NULL. Based on the existing comments
> a reader may wonder why this cannot be optimized by using kernfs_root_to_node(rdt_root)
> instead of a local variable and that would be a problem in the scenario you
> describe.
> 
> Could comment changes below help to clarify the motivation for the reference
> and provide enough information to support future changes?
> 
> 	/*
> 	 * 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;
> 	...

Looks good. Replaced previous comments with these new ones. Thanks.
> 
> Reinette
> 
-Tony