* [RFC PATCH 0/6] debugfs/wifi: locking fixes
@ 2023-11-09 21:22 Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage Johannes Berg
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Johannes Berg @ 2023-11-09 21:22 UTC (permalink / raw)
To: linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage
2023-11-09 21:22 [RFC PATCH 0/6] debugfs/wifi: locking fixes Johannes Berg
@ 2023-11-09 21:22 ` Johannes Berg
2023-11-20 11:31 ` Dan Carpenter
2023-11-09 21:22 ` [RFC PATCH 2/6] debugfs: annotate debugfs handlers vs. removal with lockdep Johannes Berg
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Johannes Berg @ 2023-11-09 21:22 UTC (permalink / raw)
To: linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
debugfs_create_automount() stores a function pointer in d_fsdata,
but since commit 7c8d469877b1 ("debugfs: add support for more
elaborate ->d_fsdata") debugfs_release_dentry() will free it, now
conditionally on DEBUGFS_FSDATA_IS_REAL_FOPS_BIT, but that's not
set for the function pointer in automount. As a result, removing
an automount dentry would attempt to free the function pointer.
Luckily, the only user of this (tracing) never removes it.
Nevertheless, it's safer if we just handle the fsdata in one way,
namely either DEBUGFS_FSDATA_IS_REAL_FOPS_BIT or allocated. Thus,
change the automount to allocate it, and use the real_fops in the
data to indicate whether or not automount is filled, rather than
adding a type tag. At least for now this isn't actually needed,
but the next changes will require it.
Also check in debugfs_file_get() that it gets only called
on regular files, just to make things clearer.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
fs/debugfs/file.c | 3 +++
fs/debugfs/inode.c | 25 ++++++++++++++++++-------
fs/debugfs/internal.h | 10 ++++++++--
3 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 87b3753aa4b1..23bdfc126b5e 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -84,6 +84,9 @@ int debugfs_file_get(struct dentry *dentry)
struct debugfs_fsdata *fsd;
void *d_fsd;
+ if (WARN_ON(!d_is_reg(dentry)))
+ return -EINVAL;
+
d_fsd = READ_ONCE(dentry->d_fsdata);
if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
fsd = d_fsd;
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 83e57e9f9fa0..269bad87d552 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -236,17 +236,19 @@ static const struct super_operations debugfs_super_operations = {
static void debugfs_release_dentry(struct dentry *dentry)
{
- void *fsd = dentry->d_fsdata;
+ struct debugfs_fsdata *fsd = dentry->d_fsdata;
- if (!((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
- kfree(dentry->d_fsdata);
+ if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
+ return;
+
+ kfree(fsd);
}
static struct vfsmount *debugfs_automount(struct path *path)
{
- debugfs_automount_t f;
- f = (debugfs_automount_t)path->dentry->d_fsdata;
- return f(path->dentry, d_inode(path->dentry)->i_private);
+ struct debugfs_fsdata *fsd = path->dentry->d_fsdata;
+
+ return fsd->automount(path->dentry, d_inode(path->dentry)->i_private);
}
static const struct dentry_operations debugfs_dops = {
@@ -634,11 +636,20 @@ struct dentry *debugfs_create_automount(const char *name,
void *data)
{
struct dentry *dentry = start_creating(name, parent);
+ struct debugfs_fsdata *fsd;
struct inode *inode;
if (IS_ERR(dentry))
return dentry;
+ fsd = kzalloc(sizeof(*fsd), GFP_KERNEL);
+ if (!fsd) {
+ failed_creating(dentry);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ fsd->automount = f;
+
if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
failed_creating(dentry);
return ERR_PTR(-EPERM);
@@ -654,7 +665,7 @@ struct dentry *debugfs_create_automount(const char *name,
make_empty_dir_inode(inode);
inode->i_flags |= S_AUTOMOUNT;
inode->i_private = data;
- dentry->d_fsdata = (void *)f;
+ dentry->d_fsdata = fsd;
/* directory inodes start off with i_nlink == 2 (for "." entry) */
inc_nlink(inode);
d_instantiate(dentry, inode);
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index 92af8ae31313..f7c489b5a368 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -17,8 +17,14 @@ extern const struct file_operations debugfs_full_proxy_file_operations;
struct debugfs_fsdata {
const struct file_operations *real_fops;
- refcount_t active_users;
- struct completion active_users_drained;
+ union {
+ /* automount_fn is used when real_fops is NULL */
+ debugfs_automount_t automount;
+ struct {
+ refcount_t active_users;
+ struct completion active_users_drained;
+ };
+ };
};
/*
--
2.41.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 2/6] debugfs: annotate debugfs handlers vs. removal with lockdep
2023-11-09 21:22 [RFC PATCH 0/6] debugfs/wifi: locking fixes Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage Johannes Berg
@ 2023-11-09 21:22 ` Johannes Berg
2023-12-02 6:37 ` Sergey Senozhatsky
2023-11-09 21:22 ` [RFC PATCH 3/6] debugfs: add API to allow debugfs operations cancellation Johannes Berg
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Johannes Berg @ 2023-11-09 21:22 UTC (permalink / raw)
To: linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
When you take a lock in a debugfs handler but also try
to remove the debugfs file under that lock, things can
deadlock since the removal has to wait for all users
to finish.
Add lockdep annotations in debugfs_file_get()/_put()
to catch such issues.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
fs/debugfs/file.c | 10 ++++++++++
fs/debugfs/inode.c | 12 ++++++++++++
fs/debugfs/internal.h | 6 ++++++
3 files changed, 28 insertions(+)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 23bdfc126b5e..e499d38b1077 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -103,6 +103,12 @@ int debugfs_file_get(struct dentry *dentry)
kfree(fsd);
fsd = READ_ONCE(dentry->d_fsdata);
}
+#ifdef CONFIG_LOCKDEP
+ fsd->lock_name = kasprintf(GFP_KERNEL, "debugfs:%pd", dentry);
+ lockdep_register_key(&fsd->key);
+ lockdep_init_map(&fsd->lockdep_map, fsd->lock_name ?: "debugfs",
+ &fsd->key, 0);
+#endif
}
/*
@@ -119,6 +125,8 @@ int debugfs_file_get(struct dentry *dentry)
if (!refcount_inc_not_zero(&fsd->active_users))
return -EIO;
+ lock_map_acquire_read(&fsd->lockdep_map);
+
return 0;
}
EXPORT_SYMBOL_GPL(debugfs_file_get);
@@ -136,6 +144,8 @@ void debugfs_file_put(struct dentry *dentry)
{
struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
+ lock_map_release(&fsd->lockdep_map);
+
if (refcount_dec_and_test(&fsd->active_users))
complete(&fsd->active_users_drained);
}
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 269bad87d552..a4c77aafb77b 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -241,6 +241,14 @@ static void debugfs_release_dentry(struct dentry *dentry)
if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
return;
+ /* check it wasn't an automount or dir */
+ if (fsd && fsd->real_fops) {
+#ifdef CONFIG_LOCKDEP
+ lockdep_unregister_key(&fsd->key);
+ kfree(fsd->lock_name);
+#endif
+ }
+
kfree(fsd);
}
@@ -742,6 +750,10 @@ static void __debugfs_file_removed(struct dentry *dentry)
fsd = READ_ONCE(dentry->d_fsdata);
if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
return;
+
+ lock_map_acquire(&fsd->lockdep_map);
+ lock_map_release(&fsd->lockdep_map);
+
if (!refcount_dec_and_test(&fsd->active_users))
wait_for_completion(&fsd->active_users_drained);
}
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index f7c489b5a368..c7d61cfc97d2 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -7,6 +7,7 @@
#ifndef _DEBUGFS_INTERNAL_H_
#define _DEBUGFS_INTERNAL_H_
+#include <linux/lockdep.h>
struct file_operations;
@@ -23,6 +24,11 @@ struct debugfs_fsdata {
struct {
refcount_t active_users;
struct completion active_users_drained;
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map lockdep_map;
+ struct lock_class_key key;
+ char *lock_name;
+#endif
};
};
};
--
2.41.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 3/6] debugfs: add API to allow debugfs operations cancellation
2023-11-09 21:22 [RFC PATCH 0/6] debugfs/wifi: locking fixes Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 2/6] debugfs: annotate debugfs handlers vs. removal with lockdep Johannes Berg
@ 2023-11-09 21:22 ` Johannes Berg
2023-11-10 9:35 ` Benjamin Berg
2023-11-09 21:22 ` [RFC PATCH 4/6] wifi: cfg80211: add locked debugfs wrappers Johannes Berg
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Johannes Berg @ 2023-11-09 21:22 UTC (permalink / raw)
To: linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear, Johannes Berg
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>
--
2.41.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 4/6] wifi: cfg80211: add locked debugfs wrappers
2023-11-09 21:22 [RFC PATCH 0/6] debugfs/wifi: locking fixes Johannes Berg
` (2 preceding siblings ...)
2023-11-09 21:22 ` [RFC PATCH 3/6] debugfs: add API to allow debugfs operations cancellation Johannes Berg
@ 2023-11-09 21:22 ` Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 6/6] wifi: mac80211: use wiphy locked debugfs for sdata/link Johannes Berg
5 siblings, 0 replies; 14+ messages in thread
From: Johannes Berg @ 2023-11-09 21:22 UTC (permalink / raw)
To: linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
Add wrappers for debugfs files that should be called with
the wiphy mutex held, while the file is also to be removed
under the wiphy mutex. This could otherwise deadlock when
a file is trying to acquire the wiphy mutex while the code
removing it holds the mutex but waits for the removal.
This actually works by pushing the execution of the read
or write handler to a wiphy work that can be cancelled
using the debugfs cancellation API.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
include/net/cfg80211.h | 46 ++++++++++++
net/wireless/debugfs.c | 160 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 206 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index b137a33a1b68..4ecfb06c413d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -9299,4 +9299,50 @@ bool cfg80211_valid_disable_subchannel_bitmap(u16 *bitmap,
*/
void cfg80211_links_removed(struct net_device *dev, u16 link_mask);
+#ifdef CONFIG_CFG80211_DEBUGFS
+/**
+ * wiphy_locked_debugfs_read - do a locked read in debugfs
+ * @wiphy: the wiphy to use
+ * @file: the file being read
+ * @buf: the buffer to fill and then read from
+ * @bufsize: size of the buffer
+ * @userbuf: the user buffer to copy to
+ * @count: read count
+ * @ppos: read position
+ * @handler: the read handler to call (under wiphy lock)
+ * @data: additional data to pass to the read handler
+ */
+ssize_t wiphy_locked_debugfs_read(struct wiphy *wiphy, struct file *file,
+ char *buf, size_t bufsize,
+ char __user *userbuf, size_t count,
+ loff_t *ppos,
+ ssize_t (*handler)(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t bufsize,
+ void *data),
+ void *data);
+
+/**
+ * wiphy_locked_debugfs_write - do a locked write in debugfs
+ * @wiphy: the wiphy to use
+ * @file: the file being written to
+ * @buf: the buffer to copy the user data to
+ * @bufsize: size of the buffer
+ * @userbuf: the user buffer to copy from
+ * @count: read count
+ * @handler: the write handler to call (under wiphy lock)
+ * @data: additional data to pass to the write handler
+ */
+ssize_t wiphy_locked_debugfs_write(struct wiphy *wiphy, struct file *file,
+ char *buf, size_t bufsize,
+ const char __user *userbuf, size_t count,
+ ssize_t (*handler)(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t count,
+ void *data),
+ void *data);
+#endif
+
#endif /* __NET_CFG80211_H */
diff --git a/net/wireless/debugfs.c b/net/wireless/debugfs.c
index 0878b162890a..40e49074e2ee 100644
--- a/net/wireless/debugfs.c
+++ b/net/wireless/debugfs.c
@@ -4,6 +4,7 @@
*
* Copyright 2009 Luis R. Rodriguez <lrodriguez@atheros.com>
* Copyright 2007 Johannes Berg <johannes@sipsolutions.net>
+ * Copyright (C) 2023 Intel Corporation
*/
#include <linux/slab.h>
@@ -109,3 +110,162 @@ void cfg80211_debugfs_rdev_add(struct cfg80211_registered_device *rdev)
DEBUGFS_ADD(long_retry_limit);
DEBUGFS_ADD(ht40allow_map);
}
+
+struct debugfs_read_work {
+ struct wiphy_work work;
+ ssize_t (*handler)(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t count,
+ void *data);
+ struct wiphy *wiphy;
+ struct file *file;
+ char *buf;
+ size_t bufsize;
+ void *data;
+ ssize_t ret;
+ struct completion completion;
+};
+
+static void wiphy_locked_debugfs_read_work(struct wiphy *wiphy,
+ struct wiphy_work *work)
+{
+ struct debugfs_read_work *w = container_of(work, typeof(*w), work);
+
+ w->ret = w->handler(w->wiphy, w->file, w->buf, w->bufsize, w->data);
+ complete(&w->completion);
+}
+
+static void wiphy_locked_debugfs_read_cancel(struct dentry *dentry,
+ void *data)
+{
+ struct debugfs_read_work *w = data;
+
+ wiphy_work_cancel(w->wiphy, &w->work);
+ complete(&w->completion);
+}
+
+ssize_t wiphy_locked_debugfs_read(struct wiphy *wiphy, struct file *file,
+ char *buf, size_t bufsize,
+ char __user *userbuf, size_t count,
+ loff_t *ppos,
+ ssize_t (*handler)(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t bufsize,
+ void *data),
+ void *data)
+{
+ struct debugfs_read_work work = {
+ .handler = handler,
+ .wiphy = wiphy,
+ .file = file,
+ .buf = buf,
+ .bufsize = bufsize,
+ .data = data,
+ .ret = -ENODEV,
+ .completion = COMPLETION_INITIALIZER_ONSTACK(work.completion),
+ };
+ struct debugfs_cancellation cancellation = {
+ .cancel = wiphy_locked_debugfs_read_cancel,
+ .cancel_data = &work,
+ };
+
+ /* don't leak stack data or whatever */
+ memset(buf, 0, bufsize);
+
+ wiphy_work_init(&work.work, wiphy_locked_debugfs_read_work);
+ wiphy_work_queue(wiphy, &work.work);
+
+ debugfs_enter_cancellation(file, &cancellation);
+ wait_for_completion(&work.completion);
+ debugfs_leave_cancellation(file, &cancellation);
+
+ if (work.ret < 0)
+ return work.ret;
+
+ if (WARN_ON(work.ret > bufsize))
+ return -EINVAL;
+
+ return simple_read_from_buffer(userbuf, count, ppos, buf, work.ret);
+}
+EXPORT_SYMBOL_GPL(wiphy_locked_debugfs_read);
+
+struct debugfs_write_work {
+ struct wiphy_work work;
+ ssize_t (*handler)(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t count,
+ void *data);
+ struct wiphy *wiphy;
+ struct file *file;
+ char *buf;
+ size_t count;
+ void *data;
+ ssize_t ret;
+ struct completion completion;
+};
+
+static void wiphy_locked_debugfs_write_work(struct wiphy *wiphy,
+ struct wiphy_work *work)
+{
+ struct debugfs_write_work *w = container_of(work, typeof(*w), work);
+
+ w->ret = w->handler(w->wiphy, w->file, w->buf, w->count, w->data);
+ complete(&w->completion);
+}
+
+static void wiphy_locked_debugfs_write_cancel(struct dentry *dentry,
+ void *data)
+{
+ struct debugfs_write_work *w = data;
+
+ wiphy_work_cancel(w->wiphy, &w->work);
+ complete(&w->completion);
+}
+
+ssize_t wiphy_locked_debugfs_write(struct wiphy *wiphy,
+ struct file *file, char *buf, size_t bufsize,
+ const char __user *userbuf, size_t count,
+ ssize_t (*handler)(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t count,
+ void *data),
+ void *data)
+{
+ struct debugfs_write_work work = {
+ .handler = handler,
+ .wiphy = wiphy,
+ .file = file,
+ .buf = buf,
+ .count = count,
+ .data = data,
+ .ret = -ENODEV,
+ .completion = COMPLETION_INITIALIZER_ONSTACK(work.completion),
+ };
+ struct debugfs_cancellation cancellation = {
+ .cancel = wiphy_locked_debugfs_write_cancel,
+ .cancel_data = &work,
+ };
+
+ /* mostly used for strings so enforce NUL-termination for safety */
+ if (count >= bufsize)
+ return -EINVAL;
+
+ memset(buf, 0, bufsize);
+
+ if (copy_from_user(buf, userbuf, count))
+ return -EFAULT;
+
+ wiphy_work_init(&work.work, wiphy_locked_debugfs_write_work);
+ wiphy_work_queue(wiphy, &work.work);
+
+ debugfs_enter_cancellation(file, &cancellation);
+ wait_for_completion(&work.completion);
+ debugfs_leave_cancellation(file, &cancellation);
+
+ return work.ret;
+}
+EXPORT_SYMBOL_GPL(wiphy_locked_debugfs_write);
--
2.41.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status
2023-11-09 21:22 [RFC PATCH 0/6] debugfs/wifi: locking fixes Johannes Berg
` (3 preceding siblings ...)
2023-11-09 21:22 ` [RFC PATCH 4/6] wifi: cfg80211: add locked debugfs wrappers Johannes Berg
@ 2023-11-09 21:22 ` Johannes Berg
2023-11-10 4:23 ` kernel test robot
2023-11-10 12:04 ` kernel test robot
2023-11-09 21:22 ` [RFC PATCH 6/6] wifi: mac80211: use wiphy locked debugfs for sdata/link Johannes Berg
5 siblings, 2 replies; 14+ messages in thread
From: Johannes Berg @ 2023-11-09 21:22 UTC (permalink / raw)
To: linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
The read is currently with RCU and the write can deadlock,
convert both for the sake of illustration.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
net/mac80211/debugfs_sta.c | 74 +++++++++++++++++++++-----------------
1 file changed, 42 insertions(+), 32 deletions(-)
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 06e3613bf46b..5bf507ebb096 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -312,23 +312,14 @@ static ssize_t sta_aql_write(struct file *file, const char __user *userbuf,
STA_OPS_RW(aql);
-static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
- size_t count, loff_t *ppos)
+static ssize_t sta_agg_status_do_read(struct wiphy *wiphy, struct file *file,
+ char *buf, size_t bufsz, void *data)
{
- char *buf, *p;
- ssize_t bufsz = 71 + IEEE80211_NUM_TIDS * 40;
+ struct sta_info *sta = data;
+ char *p = buf;
int i;
- struct sta_info *sta = file->private_data;
struct tid_ampdu_rx *tid_rx;
struct tid_ampdu_tx *tid_tx;
- ssize_t ret;
-
- buf = kzalloc(bufsz, GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
- p = buf;
-
- rcu_read_lock();
p += scnprintf(p, bufsz + buf - p, "next dialog_token: %#02x\n",
sta->ampdu_mlme.dialog_token_allocator + 1);
@@ -338,8 +329,8 @@ static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
bool tid_rx_valid;
- tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[i]);
- tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[i]);
+ tid_rx = wiphy_dereference(wiphy, sta->ampdu_mlme.tid_rx[i]);
+ tid_tx = wiphy_dereference(wiphy, sta->ampdu_mlme.tid_tx[i]);
tid_rx_valid = test_bit(i, sta->ampdu_mlme.agg_session_valid);
p += scnprintf(p, bufsz + buf - p, "%02d", i);
@@ -358,31 +349,39 @@ static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
tid_tx ? skb_queue_len(&tid_tx->pending) : 0);
p += scnprintf(p, bufsz + buf - p, "\n");
}
- rcu_read_unlock();
- ret = simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
+ return p - buf;
+}
+
+static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct sta_info *sta = file->private_data;
+ struct wiphy *wiphy = sta->local->hw.wiphy;
+ size_t bufsz = 71 + IEEE80211_NUM_TIDS * 40;
+ char *buf = kmalloc(bufsz, GFP_KERNEL);
+ ssize_t ret;
+
+ if (!buf)
+ return -ENOMEM;
+
+ ret = wiphy_locked_debugfs_read(wiphy, file, buf, bufsz,
+ userbuf, count, ppos,
+ sta_agg_status_do_read, sta);
kfree(buf);
+
return ret;
}
-static ssize_t sta_agg_status_write(struct file *file, const char __user *userbuf,
- size_t count, loff_t *ppos)
+static ssize_t sta_agg_status_do_write(struct wiphy *wiphy, struct file *file,
+ char *buf, size_t count, void *data)
{
- char _buf[25] = {}, *buf = _buf;
- struct sta_info *sta = file->private_data;
+ struct sta_info *sta = data;
bool start, tx;
unsigned long tid;
- char *pos;
+ char *pos = buf;
int ret, timeout = 5000;
- if (count > sizeof(_buf))
- return -EINVAL;
-
- if (copy_from_user(buf, userbuf, count))
- return -EFAULT;
-
- buf[sizeof(_buf) - 1] = '\0';
- pos = buf;
buf = strsep(&pos, " ");
if (!buf)
return -EINVAL;
@@ -420,7 +419,6 @@ static ssize_t sta_agg_status_write(struct file *file, const char __user *userbu
if (ret || tid >= IEEE80211_NUM_TIDS)
return -EINVAL;
- wiphy_lock(sta->local->hw.wiphy);
if (tx) {
if (start)
ret = ieee80211_start_tx_ba_session(&sta->sta, tid,
@@ -432,10 +430,22 @@ static ssize_t sta_agg_status_write(struct file *file, const char __user *userbu
3, true);
ret = 0;
}
- wiphy_unlock(sta->local->hw.wiphy);
return ret ?: count;
}
+
+static ssize_t sta_agg_status_write(struct file *file,
+ const char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct sta_info *sta = file->private_data;
+ struct wiphy *wiphy = sta->local->hw.wiphy;
+ char _buf[26];
+
+ return wiphy_locked_debugfs_write(wiphy, file, _buf, sizeof(_buf),
+ userbuf, count,
+ sta_agg_status_do_write, sta);
+}
STA_OPS_RW(agg_status);
/* link sta attributes */
--
2.41.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 6/6] wifi: mac80211: use wiphy locked debugfs for sdata/link
2023-11-09 21:22 [RFC PATCH 0/6] debugfs/wifi: locking fixes Johannes Berg
` (4 preceding siblings ...)
2023-11-09 21:22 ` [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status Johannes Berg
@ 2023-11-09 21:22 ` Johannes Berg
2023-11-10 6:33 ` kernel test robot
5 siblings, 1 reply; 14+ messages in thread
From: Johannes Berg @ 2023-11-09 21:22 UTC (permalink / raw)
To: linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
The debugfs files for netdevs (sdata) and links are removed
with the wiphy mutex held, which may deadlock. Use the new
wiphy locked debugfs to avoid that.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
net/mac80211/debugfs_netdev.c | 150 ++++++++++++++++++++++++----------
1 file changed, 105 insertions(+), 45 deletions(-)
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index ec91e131b29e..80aeb25f1b68 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -22,88 +22,148 @@
#include "debugfs_netdev.h"
#include "driver-ops.h"
+struct ieee80211_if_read_sdata_data {
+ ssize_t (*format)(const struct ieee80211_sub_if_data *, char *, int);
+ struct ieee80211_sub_if_data *sdata;
+};
+
+static ssize_t ieee80211_if_read_sdata_handler(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t bufsize,
+ void *data)
+{
+ struct ieee80211_if_read_sdata_data *d = data;
+
+ return d->format(d->sdata, buf, bufsize);
+}
+
static ssize_t ieee80211_if_read_sdata(
- struct ieee80211_sub_if_data *sdata,
+ struct file *file,
char __user *userbuf,
size_t count, loff_t *ppos,
ssize_t (*format)(const struct ieee80211_sub_if_data *sdata, char *, int))
{
+ struct ieee80211_sub_if_data *sdata = file->private_data;
+ struct ieee80211_if_read_sdata_data data = {
+ .format = format,
+ .sdata = sdata,
+ };
char buf[200];
- ssize_t ret = -EINVAL;
- wiphy_lock(sdata->local->hw.wiphy);
- ret = (*format)(sdata, buf, sizeof(buf));
- wiphy_unlock(sdata->local->hw.wiphy);
+ return wiphy_locked_debugfs_read(sdata->local->hw.wiphy,
+ file, buf, sizeof(buf),
+ userbuf, count, ppos,
+ ieee80211_if_read_sdata_handler,
+ &data);
+}
- if (ret >= 0)
- ret = simple_read_from_buffer(userbuf, count, ppos, buf, ret);
+struct ieee80211_if_write_sdata_data {
+ ssize_t (*write)(struct ieee80211_sub_if_data *, const char *, int);
+ struct ieee80211_sub_if_data *sdata;
+};
- return ret;
+static ssize_t ieee80211_if_write_sdata_handler(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t count,
+ void *data)
+{
+ struct ieee80211_if_write_sdata_data *d = data;
+
+ return d->write(d->sdata, buf, count);
}
static ssize_t ieee80211_if_write_sdata(
- struct ieee80211_sub_if_data *sdata,
+ struct file *file,
const char __user *userbuf,
size_t count, loff_t *ppos,
ssize_t (*write)(struct ieee80211_sub_if_data *sdata, const char *, int))
{
+ struct ieee80211_sub_if_data *sdata = file->private_data;
+ struct ieee80211_if_write_sdata_data data = {
+ .write = write,
+ .sdata = sdata,
+ };
char buf[64];
- ssize_t ret;
- if (count >= sizeof(buf))
- return -E2BIG;
+ return wiphy_locked_debugfs_write(sdata->local->hw.wiphy,
+ file, buf, sizeof(buf),
+ userbuf, count,
+ ieee80211_if_write_sdata_handler,
+ &data);
+}
- if (copy_from_user(buf, userbuf, count))
- return -EFAULT;
- buf[count] = '\0';
+struct ieee80211_if_read_link_data {
+ ssize_t (*format)(const struct ieee80211_link_data *, char *, int);
+ struct ieee80211_link_data *link;
+};
- wiphy_lock(sdata->local->hw.wiphy);
- ret = (*write)(sdata, buf, count);
- wiphy_unlock(sdata->local->hw.wiphy);
+static ssize_t ieee80211_if_read_link_handler(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t bufsize,
+ void *data)
+{
+ struct ieee80211_if_read_link_data *d = data;
- return ret;
+ return d->format(d->link, buf, bufsize);
}
static ssize_t ieee80211_if_read_link(
- struct ieee80211_link_data *link,
+ struct file *file,
char __user *userbuf,
size_t count, loff_t *ppos,
ssize_t (*format)(const struct ieee80211_link_data *link, char *, int))
{
+ struct ieee80211_link_data *link = file->private_data;
+ struct ieee80211_if_read_link_data data = {
+ .format = format,
+ .link = link,
+ };
char buf[200];
- ssize_t ret = -EINVAL;
- wiphy_lock(link->sdata->local->hw.wiphy);
- ret = (*format)(link, buf, sizeof(buf));
- wiphy_unlock(link->sdata->local->hw.wiphy);
+ return wiphy_locked_debugfs_read(link->sdata->local->hw.wiphy,
+ file, buf, sizeof(buf),
+ userbuf, count, ppos,
+ ieee80211_if_read_link_handler,
+ &data);
+}
- if (ret >= 0)
- ret = simple_read_from_buffer(userbuf, count, ppos, buf, ret);
+struct ieee80211_if_write_link_data {
+ ssize_t (*write)(struct ieee80211_link_data *, const char *, int);
+ struct ieee80211_link_data *link;
+};
- return ret;
+static ssize_t ieee80211_if_write_link_handler(struct wiphy *wiphy,
+ struct file *file,
+ char *buf,
+ size_t count,
+ void *data)
+{
+ struct ieee80211_if_write_sdata_data *d = data;
+
+ return d->write(d->sdata, buf, count);
}
static ssize_t ieee80211_if_write_link(
- struct ieee80211_link_data *link,
+ struct file *file,
const char __user *userbuf,
size_t count, loff_t *ppos,
ssize_t (*write)(struct ieee80211_link_data *link, const char *, int))
{
+ struct ieee80211_link_data *link = file->private_data;
+ struct ieee80211_if_write_link_data data = {
+ .write = write,
+ .link = link,
+ };
char buf[64];
- ssize_t ret;
- if (count >= sizeof(buf))
- return -E2BIG;
-
- if (copy_from_user(buf, userbuf, count))
- return -EFAULT;
- buf[count] = '\0';
-
- wiphy_lock(link->sdata->local->hw.wiphy);
- ret = (*write)(link, buf, count);
- wiphy_unlock(link->sdata->local->hw.wiphy);
-
- return ret;
+ return wiphy_locked_debugfs_write(link->sdata->local->hw.wiphy,
+ file, buf, sizeof(buf),
+ userbuf, count,
+ ieee80211_if_write_link_handler,
+ &data);
}
#define IEEE80211_IF_FMT(name, type, field, format_string) \
@@ -173,7 +233,7 @@ static ssize_t ieee80211_if_read_##name(struct file *file, \
char __user *userbuf, \
size_t count, loff_t *ppos) \
{ \
- return ieee80211_if_read_sdata(file->private_data, \
+ return ieee80211_if_read_sdata(file, \
userbuf, count, ppos, \
ieee80211_if_fmt_##name); \
}
@@ -183,7 +243,7 @@ static ssize_t ieee80211_if_write_##name(struct file *file, \
const char __user *userbuf, \
size_t count, loff_t *ppos) \
{ \
- return ieee80211_if_write_sdata(file->private_data, userbuf, \
+ return ieee80211_if_write_sdata(file, userbuf, \
count, ppos, \
ieee80211_if_parse_##name); \
}
@@ -211,7 +271,7 @@ static ssize_t ieee80211_if_read_##name(struct file *file, \
char __user *userbuf, \
size_t count, loff_t *ppos) \
{ \
- return ieee80211_if_read_link(file->private_data, \
+ return ieee80211_if_read_link(file, \
userbuf, count, ppos, \
ieee80211_if_fmt_##name); \
}
@@ -221,7 +281,7 @@ static ssize_t ieee80211_if_write_##name(struct file *file, \
const char __user *userbuf, \
size_t count, loff_t *ppos) \
{ \
- return ieee80211_if_write_link(file->private_data, userbuf, \
+ return ieee80211_if_write_link(file, userbuf, \
count, ppos, \
ieee80211_if_parse_##name); \
}
--
2.41.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status
2023-11-09 21:22 ` [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status Johannes Berg
@ 2023-11-10 4:23 ` kernel test robot
2023-11-10 12:04 ` kernel test robot
1 sibling, 0 replies; 14+ messages in thread
From: kernel test robot @ 2023-11-10 4:23 UTC (permalink / raw)
To: Johannes Berg; +Cc: oe-kbuild-all
Hi Johannes,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus wireless-next/main wireless/main linus/master v6.6 next-20231109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Berg/debugfs-fix-automount-d_fsdata-usage/20231110-054024
base: driver-core/driver-core-testing
patch link: https://lore.kernel.org/r/20231109222251.186dcaf8bdcc.Id4251db174cdd42519a5ef19cbb08d7ed8f65397%40changeid
patch subject: [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status
config: arm64-randconfig-002-20231110 (https://download.01.org/0day-ci/archive/20231110/202311101232.nfwr9Uqz-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231110/202311101232.nfwr9Uqz-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311101232.nfwr9Uqz-lkp@intel.com/
All errors (new ones prefixed by >>):
net/mac80211/debugfs_sta.c: In function 'sta_agg_status_read':
>> net/mac80211/debugfs_sta.c:368:15: error: implicit declaration of function 'wiphy_locked_debugfs_read' [-Werror=implicit-function-declaration]
368 | ret = wiphy_locked_debugfs_read(wiphy, file, buf, bufsz,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
net/mac80211/debugfs_sta.c: In function 'sta_agg_status_write':
>> net/mac80211/debugfs_sta.c:445:16: error: implicit declaration of function 'wiphy_locked_debugfs_write' [-Werror=implicit-function-declaration]
445 | return wiphy_locked_debugfs_write(wiphy, file, _buf, sizeof(_buf),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/wiphy_locked_debugfs_read +368 net/mac80211/debugfs_sta.c
355
356 static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
357 size_t count, loff_t *ppos)
358 {
359 struct sta_info *sta = file->private_data;
360 struct wiphy *wiphy = sta->local->hw.wiphy;
361 size_t bufsz = 71 + IEEE80211_NUM_TIDS * 40;
362 char *buf = kmalloc(bufsz, GFP_KERNEL);
363 ssize_t ret;
364
365 if (!buf)
366 return -ENOMEM;
367
> 368 ret = wiphy_locked_debugfs_read(wiphy, file, buf, bufsz,
369 userbuf, count, ppos,
370 sta_agg_status_do_read, sta);
371 kfree(buf);
372
373 return ret;
374 }
375
376 static ssize_t sta_agg_status_do_write(struct wiphy *wiphy, struct file *file,
377 char *buf, size_t count, void *data)
378 {
379 struct sta_info *sta = data;
380 bool start, tx;
381 unsigned long tid;
382 char *pos = buf;
383 int ret, timeout = 5000;
384
385 buf = strsep(&pos, " ");
386 if (!buf)
387 return -EINVAL;
388
389 if (!strcmp(buf, "tx"))
390 tx = true;
391 else if (!strcmp(buf, "rx"))
392 tx = false;
393 else
394 return -EINVAL;
395
396 buf = strsep(&pos, " ");
397 if (!buf)
398 return -EINVAL;
399 if (!strcmp(buf, "start")) {
400 start = true;
401 if (!tx)
402 return -EINVAL;
403 } else if (!strcmp(buf, "stop")) {
404 start = false;
405 } else {
406 return -EINVAL;
407 }
408
409 buf = strsep(&pos, " ");
410 if (!buf)
411 return -EINVAL;
412 if (sscanf(buf, "timeout=%d", &timeout) == 1) {
413 buf = strsep(&pos, " ");
414 if (!buf || !tx || !start)
415 return -EINVAL;
416 }
417
418 ret = kstrtoul(buf, 0, &tid);
419 if (ret || tid >= IEEE80211_NUM_TIDS)
420 return -EINVAL;
421
422 if (tx) {
423 if (start)
424 ret = ieee80211_start_tx_ba_session(&sta->sta, tid,
425 timeout);
426 else
427 ret = ieee80211_stop_tx_ba_session(&sta->sta, tid);
428 } else {
429 __ieee80211_stop_rx_ba_session(sta, tid, WLAN_BACK_RECIPIENT,
430 3, true);
431 ret = 0;
432 }
433
434 return ret ?: count;
435 }
436
437 static ssize_t sta_agg_status_write(struct file *file,
438 const char __user *userbuf,
439 size_t count, loff_t *ppos)
440 {
441 struct sta_info *sta = file->private_data;
442 struct wiphy *wiphy = sta->local->hw.wiphy;
443 char _buf[26];
444
> 445 return wiphy_locked_debugfs_write(wiphy, file, _buf, sizeof(_buf),
446 userbuf, count,
447 sta_agg_status_do_write, sta);
448 }
449 STA_OPS_RW(agg_status);
450
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 6/6] wifi: mac80211: use wiphy locked debugfs for sdata/link
2023-11-09 21:22 ` [RFC PATCH 6/6] wifi: mac80211: use wiphy locked debugfs for sdata/link Johannes Berg
@ 2023-11-10 6:33 ` kernel test robot
0 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2023-11-10 6:33 UTC (permalink / raw)
To: Johannes Berg; +Cc: oe-kbuild-all
Hi Johannes,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus wireless-next/main wireless/main linus/master next-20231110]
[cannot apply to v6.6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Berg/debugfs-fix-automount-d_fsdata-usage/20231110-054024
base: driver-core/driver-core-testing
patch link: https://lore.kernel.org/r/20231109222251.f6f6241e2541.I99a21601e124f5ac4e161ad32b19949e7896e390%40changeid
patch subject: [RFC PATCH 6/6] wifi: mac80211: use wiphy locked debugfs for sdata/link
config: arm64-randconfig-002-20231110 (https://download.01.org/0day-ci/archive/20231110/202311101458.nTUgKNEi-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231110/202311101458.nTUgKNEi-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311101458.nTUgKNEi-lkp@intel.com/
All errors (new ones prefixed by >>):
net/mac80211/debugfs_netdev.c: In function 'ieee80211_if_read_sdata':
>> net/mac80211/debugfs_netdev.c:54:16: error: implicit declaration of function 'wiphy_locked_debugfs_read' [-Werror=implicit-function-declaration]
54 | return wiphy_locked_debugfs_read(sdata->local->hw.wiphy,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
net/mac80211/debugfs_netdev.c: In function 'ieee80211_if_write_sdata':
>> net/mac80211/debugfs_netdev.c:90:16: error: implicit declaration of function 'wiphy_locked_debugfs_write' [-Werror=implicit-function-declaration]
90 | return wiphy_locked_debugfs_write(sdata->local->hw.wiphy,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/wiphy_locked_debugfs_read +54 net/mac80211/debugfs_netdev.c
40
41 static ssize_t ieee80211_if_read_sdata(
42 struct file *file,
43 char __user *userbuf,
44 size_t count, loff_t *ppos,
45 ssize_t (*format)(const struct ieee80211_sub_if_data *sdata, char *, int))
46 {
47 struct ieee80211_sub_if_data *sdata = file->private_data;
48 struct ieee80211_if_read_sdata_data data = {
49 .format = format,
50 .sdata = sdata,
51 };
52 char buf[200];
53
> 54 return wiphy_locked_debugfs_read(sdata->local->hw.wiphy,
55 file, buf, sizeof(buf),
56 userbuf, count, ppos,
57 ieee80211_if_read_sdata_handler,
58 &data);
59 }
60
61 struct ieee80211_if_write_sdata_data {
62 ssize_t (*write)(struct ieee80211_sub_if_data *, const char *, int);
63 struct ieee80211_sub_if_data *sdata;
64 };
65
66 static ssize_t ieee80211_if_write_sdata_handler(struct wiphy *wiphy,
67 struct file *file,
68 char *buf,
69 size_t count,
70 void *data)
71 {
72 struct ieee80211_if_write_sdata_data *d = data;
73
74 return d->write(d->sdata, buf, count);
75 }
76
77 static ssize_t ieee80211_if_write_sdata(
78 struct file *file,
79 const char __user *userbuf,
80 size_t count, loff_t *ppos,
81 ssize_t (*write)(struct ieee80211_sub_if_data *sdata, const char *, int))
82 {
83 struct ieee80211_sub_if_data *sdata = file->private_data;
84 struct ieee80211_if_write_sdata_data data = {
85 .write = write,
86 .sdata = sdata,
87 };
88 char buf[64];
89
> 90 return wiphy_locked_debugfs_write(sdata->local->hw.wiphy,
91 file, buf, sizeof(buf),
92 userbuf, count,
93 ieee80211_if_write_sdata_handler,
94 &data);
95 }
96
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/6] debugfs: add API to allow debugfs operations cancellation
2023-11-09 21:22 ` [RFC PATCH 3/6] debugfs: add API to allow debugfs operations cancellation Johannes Berg
@ 2023-11-10 9:35 ` Benjamin Berg
0 siblings, 0 replies; 14+ messages in thread
From: Benjamin Berg @ 2023-11-10 9:35 UTC (permalink / raw)
To: Johannes Berg, linux-wireless, linux-kernel
Cc: linux-fsdevel, Greg Kroah-Hartman, Rafael J. Wysocki,
Nicolai Stange, Ben Greear, Johannes Berg
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status
2023-11-09 21:22 ` [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status Johannes Berg
2023-11-10 4:23 ` kernel test robot
@ 2023-11-10 12:04 ` kernel test robot
1 sibling, 0 replies; 14+ messages in thread
From: kernel test robot @ 2023-11-10 12:04 UTC (permalink / raw)
To: Johannes Berg; +Cc: llvm, oe-kbuild-all
Hi Johannes,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus wireless-next/main wireless/main linus/master v6.6 next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Berg/debugfs-fix-automount-d_fsdata-usage/20231110-054024
base: driver-core/driver-core-testing
patch link: https://lore.kernel.org/r/20231109222251.186dcaf8bdcc.Id4251db174cdd42519a5ef19cbb08d7ed8f65397%40changeid
patch subject: [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231110/202311101917.1895GcFy-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231110/202311101917.1895GcFy-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311101917.1895GcFy-lkp@intel.com/
All errors (new ones prefixed by >>):
>> net/mac80211/debugfs_sta.c:368:8: error: call to undeclared function 'wiphy_locked_debugfs_read'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
ret = wiphy_locked_debugfs_read(wiphy, file, buf, bufsz,
^
>> net/mac80211/debugfs_sta.c:445:9: error: call to undeclared function 'wiphy_locked_debugfs_write'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
return wiphy_locked_debugfs_write(wiphy, file, _buf, sizeof(_buf),
^
2 errors generated.
vim +/wiphy_locked_debugfs_read +368 net/mac80211/debugfs_sta.c
355
356 static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
357 size_t count, loff_t *ppos)
358 {
359 struct sta_info *sta = file->private_data;
360 struct wiphy *wiphy = sta->local->hw.wiphy;
361 size_t bufsz = 71 + IEEE80211_NUM_TIDS * 40;
362 char *buf = kmalloc(bufsz, GFP_KERNEL);
363 ssize_t ret;
364
365 if (!buf)
366 return -ENOMEM;
367
> 368 ret = wiphy_locked_debugfs_read(wiphy, file, buf, bufsz,
369 userbuf, count, ppos,
370 sta_agg_status_do_read, sta);
371 kfree(buf);
372
373 return ret;
374 }
375
376 static ssize_t sta_agg_status_do_write(struct wiphy *wiphy, struct file *file,
377 char *buf, size_t count, void *data)
378 {
379 struct sta_info *sta = data;
380 bool start, tx;
381 unsigned long tid;
382 char *pos = buf;
383 int ret, timeout = 5000;
384
385 buf = strsep(&pos, " ");
386 if (!buf)
387 return -EINVAL;
388
389 if (!strcmp(buf, "tx"))
390 tx = true;
391 else if (!strcmp(buf, "rx"))
392 tx = false;
393 else
394 return -EINVAL;
395
396 buf = strsep(&pos, " ");
397 if (!buf)
398 return -EINVAL;
399 if (!strcmp(buf, "start")) {
400 start = true;
401 if (!tx)
402 return -EINVAL;
403 } else if (!strcmp(buf, "stop")) {
404 start = false;
405 } else {
406 return -EINVAL;
407 }
408
409 buf = strsep(&pos, " ");
410 if (!buf)
411 return -EINVAL;
412 if (sscanf(buf, "timeout=%d", &timeout) == 1) {
413 buf = strsep(&pos, " ");
414 if (!buf || !tx || !start)
415 return -EINVAL;
416 }
417
418 ret = kstrtoul(buf, 0, &tid);
419 if (ret || tid >= IEEE80211_NUM_TIDS)
420 return -EINVAL;
421
422 if (tx) {
423 if (start)
424 ret = ieee80211_start_tx_ba_session(&sta->sta, tid,
425 timeout);
426 else
427 ret = ieee80211_stop_tx_ba_session(&sta->sta, tid);
428 } else {
429 __ieee80211_stop_rx_ba_session(sta, tid, WLAN_BACK_RECIPIENT,
430 3, true);
431 ret = 0;
432 }
433
434 return ret ?: count;
435 }
436
437 static ssize_t sta_agg_status_write(struct file *file,
438 const char __user *userbuf,
439 size_t count, loff_t *ppos)
440 {
441 struct sta_info *sta = file->private_data;
442 struct wiphy *wiphy = sta->local->hw.wiphy;
443 char _buf[26];
444
> 445 return wiphy_locked_debugfs_write(wiphy, file, _buf, sizeof(_buf),
446 userbuf, count,
447 sta_agg_status_do_write, sta);
448 }
449 STA_OPS_RW(agg_status);
450
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage
2023-11-09 21:22 ` [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage Johannes Berg
@ 2023-11-20 11:31 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2023-11-20 11:31 UTC (permalink / raw)
To: oe-kbuild, Johannes Berg; +Cc: lkp, oe-kbuild-all
Hi Johannes,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Berg/debugfs-fix-automount-d_fsdata-usage/20231110-054024
base: driver-core/driver-core-testing
patch link: https://lore.kernel.org/r/20231109222251.9e54cb55c700.I64fe5615568e87f9ae2d7fb2ac4e5fa96924cb50%40changeid
patch subject: [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage
config: i386-randconfig-141-20231111 (https://download.01.org/0day-ci/archive/20231111/202311110653.cItFLCy6-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231111/202311110653.cItFLCy6-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202311110653.cItFLCy6-lkp@intel.com/
New smatch warnings:
fs/debugfs/inode.c:655 debugfs_create_automount() warn: possible memory leak of 'fsd'
vim +/fsd +655 fs/debugfs/inode.c
77b3da6e3232d3 Al Viro 2015-01-25 633 struct dentry *debugfs_create_automount(const char *name,
77b3da6e3232d3 Al Viro 2015-01-25 634 struct dentry *parent,
93faccbbfa958a Eric W. Biederman 2017-02-01 635 debugfs_automount_t f,
77b3da6e3232d3 Al Viro 2015-01-25 636 void *data)
77b3da6e3232d3 Al Viro 2015-01-25 637 {
77b3da6e3232d3 Al Viro 2015-01-25 638 struct dentry *dentry = start_creating(name, parent);
3e1dead436f419 Johannes Berg 2023-11-09 639 struct debugfs_fsdata *fsd;
77b3da6e3232d3 Al Viro 2015-01-25 640 struct inode *inode;
77b3da6e3232d3 Al Viro 2015-01-25 641
77b3da6e3232d3 Al Viro 2015-01-25 642 if (IS_ERR(dentry))
ff9fb72bc07705 Greg Kroah-Hartman 2019-01-23 643 return dentry;
77b3da6e3232d3 Al Viro 2015-01-25 644
3e1dead436f419 Johannes Berg 2023-11-09 645 fsd = kzalloc(sizeof(*fsd), GFP_KERNEL);
3e1dead436f419 Johannes Berg 2023-11-09 646 if (!fsd) {
3e1dead436f419 Johannes Berg 2023-11-09 647 failed_creating(dentry);
3e1dead436f419 Johannes Berg 2023-11-09 648 return ERR_PTR(-ENOMEM);
3e1dead436f419 Johannes Berg 2023-11-09 649 }
3e1dead436f419 Johannes Berg 2023-11-09 650
3e1dead436f419 Johannes Berg 2023-11-09 651 fsd->automount = f;
3e1dead436f419 Johannes Berg 2023-11-09 652
a24c6f7bc923d5 Peter Enderborg 2020-07-16 653 if (!(debugfs_allow & DEBUGFS_ALLOW_API)) {
a24c6f7bc923d5 Peter Enderborg 2020-07-16 654 failed_creating(dentry);
a24c6f7bc923d5 Peter Enderborg 2020-07-16 @655 return ERR_PTR(-EPERM);
kfree(fsd);
a24c6f7bc923d5 Peter Enderborg 2020-07-16 656 }
a24c6f7bc923d5 Peter Enderborg 2020-07-16 657
77b3da6e3232d3 Al Viro 2015-01-25 658 inode = debugfs_get_inode(dentry->d_sb);
43e23b6c0b0151 Greg Kroah-Hartman 2019-07-03 659 if (unlikely(!inode)) {
43e23b6c0b0151 Greg Kroah-Hartman 2019-07-03 660 pr_err("out of free dentries, can not create automount '%s'\n",
43e23b6c0b0151 Greg Kroah-Hartman 2019-07-03 661 name);
77b3da6e3232d3 Al Viro 2015-01-25 662 return failed_creating(dentry);
43e23b6c0b0151 Greg Kroah-Hartman 2019-07-03 663 }
77b3da6e3232d3 Al Viro 2015-01-25 664
87243deb88671f Seth Forshee 2016-03-09 665 make_empty_dir_inode(inode);
77b3da6e3232d3 Al Viro 2015-01-25 666 inode->i_flags |= S_AUTOMOUNT;
77b3da6e3232d3 Al Viro 2015-01-25 667 inode->i_private = data;
3e1dead436f419 Johannes Berg 2023-11-09 668 dentry->d_fsdata = fsd;
a8f324a46fbe54 Roman Pen 2016-02-09 669 /* directory inodes start off with i_nlink == 2 (for "." entry) */
a8f324a46fbe54 Roman Pen 2016-02-09 670 inc_nlink(inode);
77b3da6e3232d3 Al Viro 2015-01-25 671 d_instantiate(dentry, inode);
a8f324a46fbe54 Roman Pen 2016-02-09 672 inc_nlink(d_inode(dentry->d_parent));
a8f324a46fbe54 Roman Pen 2016-02-09 673 fsnotify_mkdir(d_inode(dentry->d_parent), dentry);
77b3da6e3232d3 Al Viro 2015-01-25 674 return end_creating(dentry);
77b3da6e3232d3 Al Viro 2015-01-25 675 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 2/6] debugfs: annotate debugfs handlers vs. removal with lockdep
2023-11-09 21:22 ` [RFC PATCH 2/6] debugfs: annotate debugfs handlers vs. removal with lockdep Johannes Berg
@ 2023-12-02 6:37 ` Sergey Senozhatsky
2023-12-02 10:40 ` Johannes Berg
0 siblings, 1 reply; 14+ messages in thread
From: Sergey Senozhatsky @ 2023-12-02 6:37 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless, linux-kernel, linux-fsdevel, Greg Kroah-Hartman,
Rafael J. Wysocki, Nicolai Stange, Ben Greear, Johannes Berg,
Minchan Kim
On (23/11/09 22:22), Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> When you take a lock in a debugfs handler but also try
> to remove the debugfs file under that lock, things can
> deadlock since the removal has to wait for all users
> to finish.
>
> Add lockdep annotations in debugfs_file_get()/_put()
> to catch such issues.
So this triggers when I reset zram device (zsmalloc compiled with
CONFIG_ZSMALLOC_STAT).
debugfs_create_file() and debugfs_remove_recursive() are called
under zram->init_lock, and zsmalloc never calls into zram code.
What I don't really get is where does the
`debugfs::classes -> zram->init_lock`
dependency come from?
[ 47.283364] ======================================================
[ 47.284790] WARNING: possible circular locking dependency detected
[ 47.286217] 6.7.0-rc3-next-20231201+ #239 Tainted: G N
[ 47.287723] ------------------------------------------------------
[ 47.289145] zram-test.sh/727 is trying to acquire lock:
[ 47.290350] ffff88814b824070 (debugfs:classes){++++}-{0:0}, at: remove_one+0x65/0x210
[ 47.292202]
[ 47.292202] but task is already holding lock:
[ 47.293554] ffff88812fe7ee48 (&sb->s_type->i_mutex_key#2){++++}-{3:3}, at: simple_recursive_removal+0x217/0x3a0
[ 47.295895]
[ 47.295895] which lock already depends on the new lock.
[ 47.295895]
[ 47.297757]
[ 47.297757] the existing dependency chain (in reverse order) is:
[ 47.299498]
[ 47.299498] -> #4 (&sb->s_type->i_mutex_key#2){++++}-{3:3}:
[ 47.301129] down_write+0x40/0x80
[ 47.302028] start_creating+0xa5/0x1b0
[ 47.303024] debugfs_create_dir+0x16/0x240
[ 47.304104] zs_create_pool+0x5da/0x6f0
[ 47.305123] disksize_store+0xce/0x320
[ 47.306129] kernfs_fop_write_iter+0x1cb/0x270
[ 47.307279] vfs_write+0x42f/0x4c0
[ 47.308205] ksys_write+0x8f/0x110
[ 47.309129] do_syscall_64+0x40/0xe0
[ 47.310100] entry_SYSCALL_64_after_hwframe+0x62/0x6a
[ 47.311403]
[ 47.311403] -> #3 (&zram->init_lock){++++}-{3:3}:
[ 47.312856] down_write+0x40/0x80
[ 47.313755] zram_reset_device+0x22/0x2b0
[ 47.314814] reset_store+0x15b/0x190
[ 47.315788] kernfs_fop_write_iter+0x1cb/0x270
[ 47.316946] vfs_write+0x42f/0x4c0
[ 47.317869] ksys_write+0x8f/0x110
[ 47.318787] do_syscall_64+0x40/0xe0
[ 47.319754] entry_SYSCALL_64_after_hwframe+0x62/0x6a
[ 47.321051]
[ 47.321051] -> #2 (&of->mutex){+.+.}-{3:3}:
[ 47.322374] __mutex_lock+0x97/0x810
[ 47.323338] kernfs_seq_start+0x34/0x190
[ 47.324387] seq_read_iter+0x1e1/0x6c0
[ 47.325389] vfs_read+0x38f/0x420
[ 47.326295] ksys_read+0x8f/0x110
[ 47.327200] do_syscall_64+0x40/0xe0
[ 47.328164] entry_SYSCALL_64_after_hwframe+0x62/0x6a
[ 47.329460]
[ 47.329460] -> #1 (&p->lock){+.+.}-{3:3}:
[ 47.330740] __mutex_lock+0x97/0x810
[ 47.331717] seq_read_iter+0x5c/0x6c0
[ 47.332696] seq_read+0xfe/0x140
[ 47.333577] full_proxy_read+0x90/0x110
[ 47.334598] vfs_read+0xfb/0x420
[ 47.335482] ksys_read+0x8f/0x110
[ 47.336382] do_syscall_64+0x40/0xe0
[ 47.337344] entry_SYSCALL_64_after_hwframe+0x62/0x6a
[ 47.338636]
[ 47.338636] -> #0 (debugfs:classes){++++}-{0:0}:
[ 47.340056] __lock_acquire+0x20b1/0x3b50
[ 47.341117] lock_acquire+0xe3/0x210
[ 47.342072] remove_one+0x7d/0x210
[ 47.342991] simple_recursive_removal+0x325/0x3a0
[ 47.344210] debugfs_remove+0x40/0x60
[ 47.345181] zs_destroy_pool+0x4e/0x3d0
[ 47.346199] zram_reset_device+0x151/0x2b0
[ 47.347272] reset_store+0x15b/0x190
[ 47.348257] kernfs_fop_write_iter+0x1cb/0x270
[ 47.349426] vfs_write+0x42f/0x4c0
[ 47.350350] ksys_write+0x8f/0x110
[ 47.351273] do_syscall_64+0x40/0xe0
[ 47.352239] entry_SYSCALL_64_after_hwframe+0x62/0x6a
[ 47.353536]
[ 47.353536] other info that might help us debug this:
[ 47.353536]
[ 47.355381] Chain exists of:
[ 47.355381] debugfs:classes --> &zram->init_lock --> &sb->s_type->i_mutex_key#2
[ 47.355381]
[ 47.358105] Possible unsafe locking scenario:
[ 47.358105]
[ 47.359484] CPU0 CPU1
[ 47.360545] ---- ----
[ 47.361599] lock(&sb->s_type->i_mutex_key#2);
[ 47.362665] lock(&zram->init_lock);
[ 47.364146] lock(&sb->s_type->i_mutex_key#2);
[ 47.365781] lock(debugfs:classes);
[ 47.366626]
[ 47.366626] *** DEADLOCK ***
[ 47.366626]
[ 47.368005] 5 locks held by zram-test.sh/727:
[ 47.369028] #0: ffff88811420c420 (sb_writers#5){.+.+}-{0:0}, at: vfs_write+0xff/0x4c0
[ 47.370873] #1: ffff8881346e1090 (&of->mutex){+.+.}-{3:3}, at: kernfs_fop_write_iter+0x126/0x270
[ 47.372932] #2: ffff88810b7bfb38 (kn->active#50){.+.+}-{0:0}, at: kernfs_fop_write_iter+0x137/0x270
[ 47.375052] #3: ffff88810b3e60b0 (&zram->init_lock){++++}-{3:3}, at: zram_reset_device+0x22/0x2b0
[ 47.377131] #4: ffff88812fe7ee48 (&sb->s_type->i_mutex_key#2){++++}-{3:3}, at: simple_recursive_removal+0x217/0x3a0
[ 47.379599]
[ 47.379599] stack backtrace:
[ 47.380619] CPU: 39 PID: 727 Comm: zram-test.sh Tainted: G N 6.7.0-rc3-next-20231201+ #239
[ 47.382836] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 47.384976] Call Trace:
[ 47.385565] <TASK>
[ 47.386074] dump_stack_lvl+0x6e/0xa0
[ 47.386942] check_noncircular+0x1b6/0x1e0
[ 47.387910] ? lockdep_unlock+0x96/0x130
[ 47.388838] __lock_acquire+0x20b1/0x3b50
[ 47.389789] ? d_walk+0x3c3/0x410
[ 47.390589] lock_acquire+0xe3/0x210
[ 47.391449] ? remove_one+0x65/0x210
[ 47.392301] ? preempt_count_sub+0x14/0xc0
[ 47.393264] ? _raw_spin_unlock+0x29/0x40
[ 47.394214] ? d_walk+0x3c3/0x410
[ 47.395005] ? remove_one+0x65/0x210
[ 47.395866] remove_one+0x7d/0x210
[ 47.396675] ? remove_one+0x65/0x210
[ 47.397523] ? d_invalidate+0xbe/0x170
[ 47.398412] simple_recursive_removal+0x325/0x3a0
[ 47.399521] ? debugfs_remove+0x60/0x60
[ 47.400430] debugfs_remove+0x40/0x60
[ 47.401302] zs_destroy_pool+0x4e/0x3d0
[ 47.402214] zram_reset_device+0x151/0x2b0
[ 47.403187] reset_store+0x15b/0x190
[ 47.404043] ? sysfs_kf_read+0x170/0x170
[ 47.404968] kernfs_fop_write_iter+0x1cb/0x270
[ 47.406015] vfs_write+0x42f/0x4c0
[ 47.406829] ksys_write+0x8f/0x110
[ 47.407642] do_syscall_64+0x40/0xe0
[ 47.408491] entry_SYSCALL_64_after_hwframe+0x62/0x6a
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 2/6] debugfs: annotate debugfs handlers vs. removal with lockdep
2023-12-02 6:37 ` Sergey Senozhatsky
@ 2023-12-02 10:40 ` Johannes Berg
0 siblings, 0 replies; 14+ messages in thread
From: Johannes Berg @ 2023-12-02 10:40 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-wireless, linux-kernel, linux-fsdevel, Greg Kroah-Hartman,
Rafael J. Wysocki, Nicolai Stange, Ben Greear, Minchan Kim
On Sat, 2023-12-02 at 15:37 +0900, Sergey Senozhatsky wrote:
> On (23/11/09 22:22), Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > When you take a lock in a debugfs handler but also try
> > to remove the debugfs file under that lock, things can
> > deadlock since the removal has to wait for all users
> > to finish.
> >
> > Add lockdep annotations in debugfs_file_get()/_put()
> > to catch such issues.
>
> So this triggers when I reset zram device (zsmalloc compiled with
> CONFIG_ZSMALLOC_STAT).
I shouldn't have put that into the rc, that was more or less an
accident. I think I'll do a revert.
> debugfs_create_file() and debugfs_remove_recursive() are called
> under zram->init_lock, and zsmalloc never calls into zram code.
> What I don't really get is where does the
> `debugfs::classes -> zram->init_lock`
> dependency come from?
"debugfs:classes" means a file is being accessed and "classes" is the
name, so that's
static int zs_stats_size_show(struct seq_file *s, void *v)
which uses seq_file, so there we have a seq_file lock.
I think eventually the issue is that lockdep isn't telling the various
seq_file instances apart, and you have one that's removed under lock
(classes) and another one that's taking the lock (reset).
Anyway, I'll send a revert, don't think this is ready yet. I was fixing
the wireless debugfs lockdep and had used that to debug it.
johannes
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-12-02 10:40 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-09 21:22 [RFC PATCH 0/6] debugfs/wifi: locking fixes Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 1/6] debugfs: fix automount d_fsdata usage Johannes Berg
2023-11-20 11:31 ` Dan Carpenter
2023-11-09 21:22 ` [RFC PATCH 2/6] debugfs: annotate debugfs handlers vs. removal with lockdep Johannes Berg
2023-12-02 6:37 ` Sergey Senozhatsky
2023-12-02 10:40 ` Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 3/6] debugfs: add API to allow debugfs operations cancellation Johannes Berg
2023-11-10 9:35 ` Benjamin Berg
2023-11-09 21:22 ` [RFC PATCH 4/6] wifi: cfg80211: add locked debugfs wrappers Johannes Berg
2023-11-09 21:22 ` [RFC PATCH 5/6] wifi: mac80211: use wiphy locked debugfs helpers for agg_status Johannes Berg
2023-11-10 4:23 ` kernel test robot
2023-11-10 12:04 ` kernel test robot
2023-11-09 21:22 ` [RFC PATCH 6/6] wifi: mac80211: use wiphy locked debugfs for sdata/link Johannes Berg
2023-11-10 6:33 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.