Received-SPF: pass (google.com: domain of linux-wireless+bounces-4097-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <492a290b-752f-48c3-be16-59bdf7914487@candelatech.com>
Date: Tue, 27 Feb 2024 06:51:27 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Kernel deadlock in 6.7.5 + hacks, maybe debugfs related.
To: Johannes Berg <johannes@sipsolutions.net>,
 linux-wireless <linux-wireless@vger.kernel.org>
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>
Content-Language: en-MW
From: Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <4a3edd404b84da53b56bb0a391cb42d00f42f4c5.camel@sipsolutions.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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?

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;


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com