[RFC,3/6] debugfs: add API to allow debugfs operations cancellation

Commit Message

Johannes Berg Nov. 9, 2023, 9:22 p.m. UTC

From: Johannes Berg <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Benjamin Berg Nov. 10, 2023, 9:35 a.m. UTC | #1

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 <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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>

Patch

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>