On 11/8/23 10:43, Johannes Berg wrote:
On Wed, 2023-11-08 at 09:44 -0800, Ben Greear wrote:
This method appears to actually set a S_DEAD flag in the dentry,
A *dir* entry.
so maybe
we just check that flag in the mutex_trylock failed to acquire path
in the debugfs read?
void simple_recursive_removal(struct dentry *dentry,
void (*callback)(struct dentry *))
{
struct dentry *this = dget(dentry);
while (true) {
struct dentry *victim = NULL, *child;
struct inode *inode = this->d_inode;
inode_lock(inode);
if (d_is_dir(this))
inode->i_flags |= S_DEAD;
But even if you did that, I'm not sure what you'd do? trylock and sleep,
and abort if you find it's being removed? That all sounds super awkward.
Here is my compile-only tested approach. It only tries to fix one particular
locking case in debugfs, so I'm sure it is not yet complete. I believe I
need to add that new flag in the fs.h, as S_DEAD is only for directories,
if I understand the code properly.
diff --git a/fs/libfs.c b/fs/libfs.c
index abe2b5a40ba1..5b420e019ad4 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -520,6 +520,7 @@ void simple_recursive_removal(struct dentry *dentry,
inode_lock(inode);
if (d_is_dir(this))
inode->i_flags |= S_DEAD;
+ inode->i_flags |= S_GOING_AWAY;
while ((child = find_next_child(this, victim)) == NULL) {
// kill and ascend
// update metadata while it's still locked
@@ -530,6 +531,7 @@ void simple_recursive_removal(struct dentry *dentry,
this = this->d_parent;
inode = this->d_inode;
inode_lock(inode);
+ inode->i_flags |= S_GOING_AWAY;
if (simple_positive(victim)) {
d_invalidate(victim); // avoid lost mounts
if (d_is_dir(victim))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..b76eafe31427 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2116,6 +2116,7 @@ struct super_operations {
#define S_CASEFOLD (1 << 15) /* Casefolded file */
#define S_VERITY (1 << 16) /* Verity file (using fs/verity/) */
#define S_KERNEL_FILE (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */
+#define S_GOING_AWAY (1 << 18) /* dentry is trying to be deleted */
/*
* Note that nosuid etc flags are inode-specific: setting some file-system
@@ -2163,6 +2164,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
#define IS_ENCRYPTED(inode) ((inode)->i_flags & S_ENCRYPTED)
#define IS_CASEFOLDED(inode) ((inode)->i_flags & S_CASEFOLD)
#define IS_VERITY(inode) ((inode)->i_flags & S_VERITY)
+#define IS_INODE_GOINGAWAY(inode) ((inode)->i_flags & S_GOING_AWAY)
#define IS_WHITEOUT(inode) (S_ISCHR(inode->i_mode) && \
(inode)->i_rdev == WHITEOUT_DEV)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 34fbefad7652..b4f3bf1c6b19 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5881,6 +5881,33 @@ static inline void wiphy_lock(struct wiphy *wiphy)
__acquire(&wiphy->mtx);
}
+/**
+ * wiphy_trylock - try to lock the wiphy
+ * @wiphy: the wiphy to lock
+ *
+ * This is needed around registering and unregistering netdevs that
+ * aren't created through cfg80211 calls, since that requires locking
+ * in cfg80211 when the notifiers is called, but that cannot
+ * differentiate which way it's called.
+ *
+ * It can also be used by drivers for their own purposes.
+ *
+ * When cfg80211 ops are called, the wiphy is already locked.
+ *
+ * Note that this makes sure that no workers that have been queued
+ * with wiphy_queue_work() are running.
+ *
+ * Returns 0 if lock failed, non-zero otherwise.
+ */
+static inline int wiphy_trylock(struct wiphy *wiphy)
+ __acquires(&wiphy->mtx)
+{
+ int rv = mutex_trylock(&wiphy->mtx);
+ if (rv)
+ __acquire(&wiphy->mtx);
+ return rv;
+}
+
/**
* wiphy_unlock - unlock the wiphy again
* @wiphy: the wiphy to unlock
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index ec91e131b29e..c22697969973 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -23,15 +23,31 @@
#include "driver-ops.h"
static ssize_t ieee80211_if_read_sdata(
+ struct file* file,
struct ieee80211_sub_if_data *sdata,
char __user *userbuf,
size_t count, loff_t *ppos,
ssize_t (*format)(const struct ieee80211_sub_if_data *sdata, char *, int))
{
- char buf[200];
ssize_t ret = -EINVAL;
-
- wiphy_lock(sdata->local->hw.wiphy);
+ struct inode* f_inode;
+ char buf[200];
+ int rv;
+
+ while (true) {
+ rv = wiphy_trylock(sdata->local->hw.wiphy);
+ if (rv) {
+ break; /* we have the lock */
+ } else {
+ /* failed to lock, see if we are being deleted. */
+ f_inode = file_inode(file);
+ if (IS_INODE_GOINGAWAY(f_inode) || IS_DEADDIR(f_inode)) {
+ return -ESTALE;
+ }
+ /* Sleep for a bit so we don't busy-spin too hard on try-lock */
+ msleep(1);
+ }
+ }
ret = (*format)(sdata, buf, sizeof(buf));
wiphy_unlock(sdata->local->hw.wiphy);
@@ -173,7 +189,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, file->private_data, \
userbuf, count, ppos, \
ieee80211_if_fmt_##name); \
}
Thanks,
Ben
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
