Re: Kernel deadlock in 6.7.5 + hacks, maybe debugfs related.

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2024-02-27 at 06:51 -0800, Ben Greear wrote:
> On 2/27/24 06:32, Johannes Berg wrote:
> > On Tue, 2024-02-27 at 06:29 -0800, Ben Greear wrote:
> > > > --- a/fs/debugfs/inode.c
> > > > +++ b/fs/debugfs/inode.c
> > > > @@ -751,13 +751,19 @@ static void __debugfs_file_removed(struct dentry *dentry)
> > > >    	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
> > > >    		return;
> > > >    
> > > > -	/* if we hit zero, just wait for all to finish */
> > > > -	if (!refcount_dec_and_test(&fsd->active_users)) {
> > > > -		wait_for_completion(&fsd->active_users_drained);
> > > > -		return;
> > > > -	}
> > > > +	/*
> > > > +	 * Now that debugfs_file_get() no longer sees a valid entry,
> > > > +	 * decrement the refcount to remove the initial reference.
> > > > +	 */
> > > > +	refcount_dec(&fsd->active_users);
> > 
> > > 
> > > [   94.576688] ------------[ cut here ]------------
> > > [   94.576699] refcount_t: decrement hit 0; leaking memory.
> > > 
> > 
> > Ah ... right, refcount_dec() doesn't like to hit 0, it's not meant for
> > this path.
> > 
> > I guess we can
> > 
> > if (refcount_dec_and_test(...))
> >     return;
> > 
> > while (refcount_read(...)) { ... }
> > 
> > johannes
> > 
> 
> Like this?

Yeah, looks about right. But not entirely sure I'm thinking this through
correctly right now ...

> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 034a617cb1a5..166053095610 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -751,13 +751,20 @@ static void __debugfs_file_removed(struct dentry *dentry)
>          if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
>                  return;
> 
> -       /* if we hit zero, just wait for all to finish */
> -       if (!refcount_dec_and_test(&fsd->active_users)) {
> -               wait_for_completion(&fsd->active_users_drained);
> +       /*
> +        * Now that debugfs_file_get() no longer sees a valid entry,
> +        * decrement the refcount to remove the initial reference.
> +        */
> +       if (!refcount_dec_and_test(&fsd->active_users))
>                  return;
> -       }
> 
> -       /* if we didn't hit zero, try to cancel any we can */
> +       /*
> +        * As long as it's not zero, try to cancel any cancellations,
> +        * new incoming ones will wake up the completion as we might
> +        * have raced: debugfs_file_get() had already been done, but
> +        * debugfs_enter_cancellation() hadn't, by the time we got
> +        * to this point here.
> +        */
>          while (refcount_read(&fsd->active_users)) {
>                  struct debugfs_cancellation *c;
> 
>