From patchwork Thu Nov 9 21:22:52 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Johannes Berg X-Patchwork-Id: 13451767 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 72A62C4332F for ; Thu, 9 Nov 2023 21:38:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230150AbjKIViW (ORCPT ); Thu, 9 Nov 2023 16:38:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42242 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229630AbjKIViV (ORCPT ); Thu, 9 Nov 2023 16:38:21 -0500 Received: from sipsolutions.net (s3.sipsolutions.net [IPv6:2a01:4f8:242:246e::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3FC942139; Thu, 9 Nov 2023 13:38:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sipsolutions.net; s=mail; h=Content-Transfer-Encoding:MIME-Version: Message-ID:Date:Subject:Cc:To:From:Content-Type:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-To:Resent-Cc: Resent-Message-ID:In-Reply-To:References; bh=jTULZb7nPWSXQoAz/suqF1eRILvjbvT5JeWBGWqkoIs=; t=1699565899; x=1700775499; b=yWmkr2DsB9ml6dHBZGwupNJRRGnOI4LA7w/RsvrdVvbpU0hOGjX5qSby3a6fHhpyASv7IafZ15J 5mkvjIPmuhPY42jaSAInV4G4T0B0Y6YPyvvig7mVIQu8JwltvZeVAqfluE5nzLLJp2MHZ+GKQrVFv 0qNGrz2JflJBlIg8rG59zhhbtcQWzjxHB81zytmFBkAt9rDwD6KiPiBT2YDE1iYFf8eKofw5wLryH rZ/sGHqxXGKAS7BpPXjtQrosw2VB0CJUs3lK/qm6FM3w6QB0/7sajAngVCnIODvmKsj/6jrl/L/J2 ve/qrKph1rOPwKiYth19qoKbhX7mDoDpW6GA==; Received: by sipsolutions.net with esmtpsa (TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.97) (envelope-from ) id 1r1CjA-00000001znF-2PA8; Thu, 09 Nov 2023 22:38:16 +0100 From: Johannes Berg To: linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, Greg Kroah-Hartman , "Rafael J. Wysocki" , Nicolai Stange , Ben Greear Subject: [RFC PATCH 0/6] debugfs/wifi: locking fixes Date: Thu, 9 Nov 2023 22:22:52 +0100 Message-ID: <20231109212251.213873-7-johannes@sipsolutions.net> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org Hi, So ... this is a bit complex. Ben found [1] that since the wireless locking rework [2] we have a deadlock in the debugfs use in the wireless stack, since some objects (netdevs, links, stations) can be removed while holding the wiphy->mtx, where as the files (all netdev/link and agg_status for stations) also acquire the wiphy->mtx. This of course leads to deadlocks, since Nicolai's proxy_fops work [3] we wait for the users of the file to finish before removing them, which they cannot in this case: thread 1 thread 2 lock(wiphy->mutex) read()/write() -> lock(wiphy->mutex) // waits for mutex debugfs_remove() -> wait_for_users() // cannot finish Unfortunately, there's no good way to remove the debugfs files *before* locking in wireless, since we may not even know which object needs to get removed, etc. Also, some files may need to be removed based on other actions, and we want/need to free the objects. I went back and forth on how to fix it, and Ben had some hacks in the threads, but in the end I decided on the approach taken in this patchset. So I have * debugfs: fix automount d_fsdata usage This patch fixes a bug in the existing automount case in debugfs, if that dentry were ever removed, we'd try to kfree() the function pointer. I previously tried to fix this differently [4] but that doesn't work, so here I just allocate a debugfs fsdata for automount, there's a single instance of this in the tree ... * debugfs: annotate debugfs handlers vs. removal with lockdep This just makes the problem obvious, whether in wireless or elsewhere, by annotating it with lockdep so that we get complaints about the deadlock described above. I've checked that the deadlock in wireless actually gets reported. * debugfs: add API to allow debugfs operations cancellation This adds a bit of infrastructure in debugfs to allow for cancellation of read/write/... handlers when remove() is called for a file. The API is written more generically, so that it could also be used to e.g. cancel operations in hardware/firmware, for example, if debugfs does accesses to that. * wifi: cfg80211: add locked debugfs wrappers I went back and forth on this, but in the end this seemed the easiest approach. Using these new helpers from debugfs files that are removed under the wiphy lock is safe, at the expense of pushing the read/write functions into a new wiphy work, which is called with wiphy->mutex held. This then uses the debugfs API introduced in the previous patch to cancel operations that are pending while the file is removed. * wifi: mac80211: use wiphy locked debugfs helpers for agg_status * wifi: mac80211: use wiphy locked debugfs for sdata/link These convert the files that actually have the problem in mac80211 to use the new helpers. Any comments would be appreciated :-) [1] https://lore.kernel.org/r/56d0b043-0585-5380-5703-f25d9a42f39d@candelatech.com [2] in particular commit 0ab6cba0696d ("wifi: mac80211: hold wiphy lock in netdev/link debugfs") but there's a lot more work that went into it [3] commit e9117a5a4bf6 ("debugfs: implement per-file removal protection") [4] https://lore.kernel.org/lkml/20231109160639.514a2568f1e7.I64fe5615568e87f9ae2d7fb2ac4e5fa96924cb50@changeid/ johannes