| Message ID | 20231109212251.213873-7-johannes@sipsolutions.net |
|---|---|
| Headers | show |
| Series | debugfs/wifi: locking fixes | expand |
Hi,
I thought about this a bit more, and I think we need to change it
slightly to hold a lock around the cancellation call. After all, it is
an important guarantee that the callback has completed before
debugfs_leave_cancellation() returns.
thread 1 thread 2
read()/write()
__debugfs_file_removed()
wait_for_completion()
debugfs_enter_cancellation()
complete()
cancel_callback starts
debugfs_leave_cancellation()
-> stack data goes out of scope
debugfs_put_file()
cancel_callback returns
Benjamin
On Thu, 2023-11-09 at 22:22 +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> In some cases there might be longer-running hardware accesses
> in debugfs files, or attempts to acquire locks, and we want
> to still be able to quickly remove the files.
>
> Introduce a cancellations API to use inside the debugfs handler
> functions to be able to cancel such operations on a per-file
> basis.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> fs/debugfs/file.c | 82
> +++++++++++++++++++++++++++++++++++++++++
> fs/debugfs/inode.c | 23 +++++++++++-
> fs/debugfs/internal.h | 5 +++
> include/linux/debugfs.h | 19 ++++++++++
> 4 files changed, 128 insertions(+), 1 deletion(-)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index e499d38b1077..f6993c068322 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -109,6 +109,8 @@ int debugfs_file_get(struct dentry *dentry)
> lockdep_init_map(&fsd->lockdep_map, fsd->lock_name ?:
> "debugfs",
> &fsd->key, 0);
> #endif
> + INIT_LIST_HEAD(&fsd->cancellations);
> + spin_lock_init(&fsd->cancellations_lock);
> }
>
> /*
> @@ -151,6 +153,86 @@ void debugfs_file_put(struct dentry *dentry)
> }
> EXPORT_SYMBOL_GPL(debugfs_file_put);
>
> +/**
> + * debugfs_enter_cancellation - enter a debugfs cancellation
> + * @file: the file being accessed
> + * @cancellation: the cancellation object, the cancel callback
> + * inside of it must be initialized
> + *
> + * When a debugfs file is removed it needs to wait for all active
> + * operations to complete. However, the operation itself may need
> + * to wait for hardware or completion of some asynchronous process
> + * or similar. As such, it may need to be cancelled to avoid long
> + * waits or even deadlocks.
> + *
> + * This function can be used inside a debugfs handler that may
> + * need to be cancelled. As soon as this function is called, the
> + * cancellation's 'cancel' callback may be called, at which point
> + * the caller should proceed to call debugfs_leave_cancellation()
> + * and leave the debugfs handler function as soon as possible.
> + * Note that the 'cancel' callback is only ever called in the
> + * context of some kind of debugfs_remove().
> + *
> + * This function must be paired with debugfs_leave_cancellation().
> + */
> +void debugfs_enter_cancellation(struct file *file,
> + struct debugfs_cancellation
> *cancellation)
> +{
> + struct debugfs_fsdata *fsd;
> + struct dentry *dentry = F_DENTRY(file);
> +
> + INIT_LIST_HEAD(&cancellation->list);
> +
> + if (WARN_ON(!d_is_reg(dentry)))
> + return;
> +
> + if (WARN_ON(!cancellation->cancel))
> + return;
> +
> + fsd = READ_ONCE(dentry->d_fsdata);
> + if (WARN_ON(!fsd ||
> + ((unsigned long)fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
> + return;
> +
> + spin_lock(&fsd->cancellations_lock);
> + list_add(&cancellation->list, &fsd->cancellations);
> + spin_unlock(&fsd->cancellations_lock);
> +
> + /* if we're already removing wake it up to cancel */
> + if (d_unlinked(dentry))
> + complete(&fsd->active_users_drained);
> +}
> +EXPORT_SYMBOL_GPL(debugfs_enter_cancellation);
> +
> +/**
> + * debugfs_leave_cancellation - leave cancellation section
> + * @file: the file being accessed
> + * @cancellation: the cancellation previously registered with
> + * debugfs_enter_cancellation()
> + *
> + * See the documentation of debugfs_enter_cancellation().
> + */
> +void debugfs_leave_cancellation(struct file *file,
> + struct debugfs_cancellation
> *cancellation)
> +{
> + struct debugfs_fsdata *fsd;
> + struct dentry *dentry = F_DENTRY(file);
> +
> + if (WARN_ON(!d_is_reg(dentry)))
> + return;
> +
> + fsd = READ_ONCE(dentry->d_fsdata);
> + if (WARN_ON(!fsd ||
> + ((unsigned long)fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
> + return;
> +
> + spin_lock(&fsd->cancellations_lock);
> + if (!list_empty(&cancellation->list))
> + list_del(&cancellation->list);
> + spin_unlock(&fsd->cancellations_lock);
> +}
> +EXPORT_SYMBOL_GPL(debugfs_leave_cancellation);
> +
> /*
> * Only permit access to world-readable files when the kernel is
> locked down.
> * We also need to exclude any file that has ways to write or alter
> it as root
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index a4c77aafb77b..2cbcc49a8826 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -247,6 +247,7 @@ static void debugfs_release_dentry(struct dentry
> *dentry)
> lockdep_unregister_key(&fsd->key);
> kfree(fsd->lock_name);
> #endif
> + WARN_ON(!list_empty(&fsd->cancellations));
> }
>
> kfree(fsd);
> @@ -754,8 +755,28 @@ static void __debugfs_file_removed(struct dentry
> *dentry)
> lock_map_acquire(&fsd->lockdep_map);
> lock_map_release(&fsd->lockdep_map);
>
> - if (!refcount_dec_and_test(&fsd->active_users))
> + /* 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;
> + }
> +
> + /* if we didn't hit zero, try to cancel any we can */
> + while (refcount_read(&fsd->active_users)) {
> + struct debugfs_cancellation *c;
> +
> + spin_lock(&fsd->cancellations_lock);
> + while ((c = list_first_entry_or_null(&fsd-
> >cancellations,
> + typeof(*c),
> list))) {
> + list_del_init(&c->list);
> + spin_unlock(&fsd->cancellations_lock);
> + c->cancel(dentry, c->cancel_data);
> + spin_lock(&fsd->cancellations_lock);
> + }
> + spin_unlock(&fsd->cancellations_lock);
> +
> + wait_for_completion(&fsd->active_users_drained);
> + }
> }
>
> static void remove_one(struct dentry *victim)
> diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
> index c7d61cfc97d2..5f279abd9351 100644
> --- a/fs/debugfs/internal.h
> +++ b/fs/debugfs/internal.h
> @@ -8,6 +8,7 @@
> #ifndef _DEBUGFS_INTERNAL_H_
> #define _DEBUGFS_INTERNAL_H_
> #include <linux/lockdep.h>
> +#include <linux/list.h>
>
> struct file_operations;
>
> @@ -29,6 +30,10 @@ struct debugfs_fsdata {
> struct lock_class_key key;
> char *lock_name;
> #endif
> +
> + /* lock for the cancellations list */
> + spinlock_t cancellations_lock;
> + struct list_head cancellations;
> };
> };
> };
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index ea2d919fd9c7..c9c65b132c0f 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -171,6 +171,25 @@ ssize_t debugfs_write_file_bool(struct file
> *file, const char __user *user_buf,
> ssize_t debugfs_read_file_str(struct file *file, char __user
> *user_buf,
> size_t count, loff_t *ppos);
>
> +/**
> + * struct debugfs_cancellation - cancellation data
> + * @list: internal, for keeping track
> + * @cancel: callback to call
> + * @cancel_data: extra data for the callback to call
> + */
> +struct debugfs_cancellation {
> + struct list_head list;
> + void (*cancel)(struct dentry *, void *);
> + void *cancel_data;
> +};
> +
> +void __acquires(cancellation)
> +debugfs_enter_cancellation(struct file *file,
> + struct debugfs_cancellation
> *cancellation);
> +void __releases(cancellation)
> +debugfs_leave_cancellation(struct file *file,
> + struct debugfs_cancellation
> *cancellation);
> +
> #else
>
> #include <linux/err.h>
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