Received-SPF: pass (google.com: domain of linux-wireless+bounces-4101-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <366ed520b5f4b3cd603be11124f9b51e32ce8106.camel@sipsolutions.net>
Subject: Re: Kernel deadlock in 6.7.5 + hacks, maybe debugfs related.
From: Johannes Berg <johannes@sipsolutions.net>
To: Ben Greear <greearb@candelatech.com>, linux-wireless
	 <linux-wireless@vger.kernel.org>
Date: Tue, 27 Feb 2024 15:58:35 +0100
In-Reply-To: <492a290b-752f-48c3-be16-59bdf7914487@candelatech.com>
References: <1c9fa9e5-09f1-0522-fdbc-dbcef4d255ca@candelatech.com>
	 <bdc7c51e3b6afaa7ac79efa15c6d1d41e9e918b5.camel@sipsolutions.net>
	 <95d56b9a-4b91-476f-bab3-7af0f28ff469@candelatech.com>
	 <0f8a0451e547f318febcbe36823e1f9914cc684b.camel@sipsolutions.net>
	 <33cc22b8-5852-42ae-9e80-8c3c58c36b6d@candelatech.com>
	 <4a3edd404b84da53b56bb0a391cb42d00f42f4c5.camel@sipsolutions.net>
	 <492a290b-752f-48c3-be16-59bdf7914487@candelatech.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

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 den=
try *dentry)
> > > >    	if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
> > > >    		return;
> > > >   =20
> > > > -	/* 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);
> >=20
> > >=20
> > > [   94.576688] ------------[ cut here ]------------
> > > [   94.576699] refcount_t: decrement hit 0; leaking memory.
> > >=20
> >=20
> > Ah ... right, refcount_dec() doesn't like to hit 0, it's not meant for
> > this path.
> >=20
> > I guess we can
> >=20
> > if (refcount_dec_and_test(...))
> >     return;
> >=20
> > while (refcount_read(...)) { ... }
> >=20
> > johannes
> >=20
>=20
> Like this?
>=20
> 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 *d=
entry)
>          if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
>                  return;
>=20
> -       /* 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;
> -       }
>=20

Which, btw, ignoring comments, braces, whitespace - then really just
removes the line you're getting stuck on.

So actually no ... invert the test?

 if (refcount_dec_and_test(...))
   return;

If it hit zero here, there's guaranteed to be no user, so we can return.

If it's not zero yet, we might yet go into a new cancellation, so we
need the rest of the function.

johannes