Received-SPF: pass (google.com: domain of linux-wireless+bounces-4108-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <70e92b03-1566-7eff-ba28-18ef69785205@candelatech.com>
Date: Tue, 27 Feb 2024 09:25:02 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: Kernel deadlock in 6.7.5 + hacks, maybe debugfs related.
Content-Language: en-US
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>
 <492a290b-752f-48c3-be16-59bdf7914487@candelatech.com>
 <366ed520b5f4b3cd603be11124f9b51e32ce8106.camel@sipsolutions.net>
 <f2aec39d67cdb20cf813b575231ab95b409e315d.camel@sipsolutions.net>
From: Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <f2aec39d67cdb20cf813b575231ab95b409e315d.camel@sipsolutions.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2/27/24 08:13, Johannes Berg wrote:
> On Tue, 2024-02-27 at 15:58 +0100, Johannes Berg wrote:
>>
>> 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.
>>
> 
> This is what I wrote now:
> 
> 
> Subject: [PATCH] debugfs: fix wait/cancellation handling during remove
> 
> Ben Greear further reports deadlocks during concurrent debugfs
> remove while files are being accessed, even though the code in
> question now uses debugfs cancellations. Turns out that despite
> all the review on the locking, we missed completely that the
> logic is wrong: if the refcount hits zero we can finish (and
> need not wait for the completion), but if it doesn't we have
> to trigger all the cancellations. As written, we can _never_
> get into the loop triggering the cancellations. Fix this, and
> explain it better while at it.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8c88a474357e ("debugfs: add API to allow debugfs operations cancellation")
> Reported-by: Ben Greear <greearb@candelatech.com>
> Closes: https://lore.kernel.org/r/1c9fa9e5-09f1-0522-fdbc-dbcef4d255ca@candelatech.com
> Change-Id: I6c7aeff8c9d6628a8bc1ddcf332205a49d801f17
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   fs/debugfs/inode.c | 25 ++++++++++++++++++++-----
>   1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 034a617cb1a5..a40da0065433 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -751,13 +751,28 @@ 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);
> +	/* if this was the last reference, we're done */
> +	if (refcount_dec_and_test(&fsd->active_users))
>   		return;
> -	}
>   
> -	/* if we didn't hit zero, try to cancel any we can */
> +	/*
> +	 * If there's still a reference, the code that obtained it can
> +	 * be in different states:
> +	 *  - The common case of not using cancellations, or already
> +	 *    after debugfs_leave_cancellation(), where we just need
> +	 *    to wait for debugfs_file_put() which signals the completion;
> +	 *  - inside a cancellation section, i.e. between
> +	 *    debugfs_enter_cancellation() and debugfs_leave_cancellation(),
> +	 *    in which case we need to trigger the ->cancel() function,
> +	 *    and then wait for debugfs_file_put() just like in the
> +	 *    previous case;
> +	 *  - before debugfs_enter_cancellation() (but obviously after
> +	 *    debugfs_file_get()), in which case we may not see the
> +	 *    cancellation in the list on the first round of the loop,
> +	 *    but debugfs_enter_cancellation() signals the completion
> +	 *    after adding it, so this code gets woken up to call the
> +	 *    ->cancel() function.
> +	 */
>   	while (refcount_read(&fsd->active_users)) {
>   		struct debugfs_cancellation *c;
>   
> 
> 
> Can you test it and let me know if that works?
> 
> Same as what we discussed, FWIW, other than the comments.

Thanks for the patch, I'm adding it to our kernel now and we'll start testing it.

Thanks,
Ben

> 
> johannes
> 

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