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

[Reinette Chatre]

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;
	...

Reinette