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

[Luck, Tony]

On Wed, May 13, 2026 at 11:24:47AM +0800, Chen, Yu C wrote:
> Hi Reinette,
> On 5/12/2026 10:34 PM, Reinette Chatre wrote:
> > Hi Chenyu,
> > 
> > On 5/12/26 12:28 AM, Chen, Yu C wrote:
> > > On 5/12/2026 6:53 AM, Reinette Chatre wrote:
> > > > Hi Tony,
> > > > 
> > > > On 5/8/26 11:21 AM, Tony Luck wrote:
> > > 
> > > > +     * Obtain reference with locks held to protect against interference
> > > > +     * from resctrl_exit().
> > > > +    */
> > > > +    kernfs_get(rdt_root_kn);
> > > 
> > > [ ... ]
> > > 
> > > > @@ -3130,6 +3144,7 @@ static int rdt_get_tree(struct fs_context *fc)
> > > >         */
> > > >        if (!ctx->kfc.new_sb_created)
> > > >            resctrl_unmount();
> > > > +    kernfs_put(rdt_root_kn);
> > > 
> > > I wonder if above should be protected against
> > >      cpus_read_lock();
> > >      mutex_lock(&rdtgroup_mutex);
> > > like kernfs_get()?
> > 
> > It is not obvious to me what this protection would be needed for.
> > Do you have a troublesome scenario in mind?
> > 
> > rdt_root_kn is a local copy of rdtgroup_default.kn. The latter is indeed
> > protected by the mutex. The reason why the kernfs_get() is protected
> > by the mutex is to ensure what rdt_root_kn points to, rdtgroup_default.kn, remains
> > accessible after the mutex is dropped. Nothing else modifies rdt_root_kn. I
> > understand the appeal of symmetry but it is not clear to me what the extra
> > locking is needed for here?
> > 
> 
> Thanks for the detailed explanation. I now agree there is no need to
> protect kernfs_put() with a lock here only for symmetry reason. I
> previously thought racing conditions would occur if two code paths
> concurrently enter kernfs_put() and target the same data area.
> However, since kernfs_put() contains an atomic compare, only one
> code path can proceed, making the operation safe.
> 
> > Could it perhaps make this flow easier to understand if the kernfs_get() is
> > of the mutex protected rdtgroup_default.kn while the kernfs_put() is
> > of the local backup copy? For example:
> > 
> > 	/* 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.
> > 	 /
> 
> Yes, this comment is very clear and helpful.
> 
> thanks,
> Chenyu

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;

-Tony