* [for-next][PATCH 01/12] tracing: Have trace_event_file have ref counters
[not found] <20231101213718.381015321@goodmis.org>
@ 2023-11-01 21:37 ` Steven Rostedt
2023-11-01 21:37 ` [for-next][PATCH 03/12] tracing: Have the user copy of synthetic event address use correct context Steven Rostedt
` (3 subsequent siblings)
4 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2023-11-01 21:37 UTC (permalink / raw)
To: linux-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, stable,
Beau Belgrave
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The following can crash the kernel:
# cd /sys/kernel/tracing
# echo 'p:sched schedule' > kprobe_events
# exec 5>>events/kprobes/sched/enable
# > kprobe_events
# exec 5>&-
The above commands:
1. Change directory to the tracefs directory
2. Create a kprobe event (doesn't matter what one)
3. Open bash file descriptor 5 on the enable file of the kprobe event
4. Delete the kprobe event (removes the files too)
5. Close the bash file descriptor 5
The above causes a crash!
BUG: kernel NULL pointer dereference, address: 0000000000000028
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP PTI
CPU: 6 PID: 877 Comm: bash Not tainted 6.5.0-rc4-test-00008-g2c6b6b1029d4-dirty #186
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:tracing_release_file_tr+0xc/0x50
What happens here is that the kprobe event creates a trace_event_file
"file" descriptor that represents the file in tracefs to the event. It
maintains state of the event (is it enabled for the given instance?).
Opening the "enable" file gets a reference to the event "file" descriptor
via the open file descriptor. When the kprobe event is deleted, the file is
also deleted from the tracefs system which also frees the event "file"
descriptor.
But as the tracefs file is still opened by user space, it will not be
totally removed until the final dput() is called on it. But this is not
true with the event "file" descriptor that is already freed. If the user
does a write to or simply closes the file descriptor it will reference the
event "file" descriptor that was just freed, causing a use-after-free bug.
To solve this, add a ref count to the event "file" descriptor as well as a
new flag called "FREED". The "file" will not be freed until the last
reference is released. But the FREE flag will be set when the event is
removed to prevent any more modifications to that event from happening,
even if there's still a reference to the event "file" descriptor.
Link: https://lore.kernel.org/linux-trace-kernel/20231031000031.1e705592@gandalf.local.home/
Link: https://lore.kernel.org/linux-trace-kernel/20231031122453.7a48b923@gandalf.local.home
Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Fixes: f5ca233e2e66d ("tracing: Increase trace array ref count on enable and filter files")
Reported-by: Beau Belgrave <beaub@linux.microsoft.com>
Tested-by: Beau Belgrave <beaub@linux.microsoft.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
include/linux/trace_events.h | 4 ++++
kernel/trace/trace.c | 15 +++++++++++++++
kernel/trace/trace.h | 3 +++
kernel/trace/trace_events.c | 31 ++++++++++++++++++++++++++----
kernel/trace/trace_events_filter.c | 3 +++
5 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 12207dc6722d..696f8dc4aa53 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -492,6 +492,7 @@ enum {
EVENT_FILE_FL_TRIGGER_COND_BIT,
EVENT_FILE_FL_PID_FILTER_BIT,
EVENT_FILE_FL_WAS_ENABLED_BIT,
+ EVENT_FILE_FL_FREED_BIT,
};
extern struct trace_event_file *trace_get_event_file(const char *instance,
@@ -630,6 +631,7 @@ extern int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...);
* TRIGGER_COND - When set, one or more triggers has an associated filter
* PID_FILTER - When set, the event is filtered based on pid
* WAS_ENABLED - Set when enabled to know to clear trace on module removal
+ * FREED - File descriptor is freed, all fields should be considered invalid
*/
enum {
EVENT_FILE_FL_ENABLED = (1 << EVENT_FILE_FL_ENABLED_BIT),
@@ -643,6 +645,7 @@ enum {
EVENT_FILE_FL_TRIGGER_COND = (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
EVENT_FILE_FL_PID_FILTER = (1 << EVENT_FILE_FL_PID_FILTER_BIT),
EVENT_FILE_FL_WAS_ENABLED = (1 << EVENT_FILE_FL_WAS_ENABLED_BIT),
+ EVENT_FILE_FL_FREED = (1 << EVENT_FILE_FL_FREED_BIT),
};
struct trace_event_file {
@@ -671,6 +674,7 @@ struct trace_event_file {
* caching and such. Which is mostly OK ;-)
*/
unsigned long flags;
+ atomic_t ref; /* ref count for opened files */
atomic_t sm_ref; /* soft-mode reference counter */
atomic_t tm_ref; /* trigger-mode reference counter */
};
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2539cfc20a97..9aebf904ff97 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4978,6 +4978,20 @@ int tracing_open_file_tr(struct inode *inode, struct file *filp)
if (ret)
return ret;
+ mutex_lock(&event_mutex);
+
+ /* Fail if the file is marked for removal */
+ if (file->flags & EVENT_FILE_FL_FREED) {
+ trace_array_put(file->tr);
+ ret = -ENODEV;
+ } else {
+ event_file_get(file);
+ }
+
+ mutex_unlock(&event_mutex);
+ if (ret)
+ return ret;
+
filp->private_data = inode->i_private;
return 0;
@@ -4988,6 +5002,7 @@ int tracing_release_file_tr(struct inode *inode, struct file *filp)
struct trace_event_file *file = inode->i_private;
trace_array_put(file->tr);
+ event_file_put(file);
return 0;
}
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 0e1405abf4f7..b7f4ea25a194 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1669,6 +1669,9 @@ extern void event_trigger_unregister(struct event_command *cmd_ops,
char *glob,
struct event_trigger_data *trigger_data);
+extern void event_file_get(struct trace_event_file *file);
+extern void event_file_put(struct trace_event_file *file);
+
/**
* struct event_trigger_ops - callbacks for trace event triggers
*
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index f9e3e24d8796..f29e815ca5b2 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -990,13 +990,35 @@ static void remove_subsystem(struct trace_subsystem_dir *dir)
}
}
+void event_file_get(struct trace_event_file *file)
+{
+ atomic_inc(&file->ref);
+}
+
+void event_file_put(struct trace_event_file *file)
+{
+ if (WARN_ON_ONCE(!atomic_read(&file->ref))) {
+ if (file->flags & EVENT_FILE_FL_FREED)
+ kmem_cache_free(file_cachep, file);
+ return;
+ }
+
+ if (atomic_dec_and_test(&file->ref)) {
+ /* Count should only go to zero when it is freed */
+ if (WARN_ON_ONCE(!(file->flags & EVENT_FILE_FL_FREED)))
+ return;
+ kmem_cache_free(file_cachep, file);
+ }
+}
+
static void remove_event_file_dir(struct trace_event_file *file)
{
eventfs_remove_dir(file->ei);
list_del(&file->list);
remove_subsystem(file->system);
free_event_filter(file->filter);
- kmem_cache_free(file_cachep, file);
+ file->flags |= EVENT_FILE_FL_FREED;
+ event_file_put(file);
}
/*
@@ -1369,7 +1391,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
flags = file->flags;
mutex_unlock(&event_mutex);
- if (!file)
+ if (!file || flags & EVENT_FILE_FL_FREED)
return -ENODEV;
if (flags & EVENT_FILE_FL_ENABLED &&
@@ -1403,7 +1425,7 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
ret = -ENODEV;
mutex_lock(&event_mutex);
file = event_file_data(filp);
- if (likely(file)) {
+ if (likely(file && !(file->flags & EVENT_FILE_FL_FREED))) {
ret = tracing_update_buffers(file->tr);
if (ret < 0) {
mutex_unlock(&event_mutex);
@@ -1683,7 +1705,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
mutex_lock(&event_mutex);
file = event_file_data(filp);
- if (file)
+ if (file && !(file->flags & EVENT_FILE_FL_FREED))
print_event_filter(file, s);
mutex_unlock(&event_mutex);
@@ -2902,6 +2924,7 @@ trace_create_new_event(struct trace_event_call *call,
atomic_set(&file->tm_ref, 0);
INIT_LIST_HEAD(&file->triggers);
list_add(&file->list, &tr->events);
+ event_file_get(file);
return file;
}
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 33264e510d16..0c611b281a5b 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -2349,6 +2349,9 @@ int apply_event_filter(struct trace_event_file *file, char *filter_string)
struct event_filter *filter = NULL;
int err;
+ if (file->flags & EVENT_FILE_FL_FREED)
+ return -ENODEV;
+
if (!strcmp(strstrip(filter_string), "0")) {
filter_disable(file);
filter = event_filter(file);
--
2.42.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [for-next][PATCH 03/12] tracing: Have the user copy of synthetic event address use correct context
[not found] <20231101213718.381015321@goodmis.org>
2023-11-01 21:37 ` [for-next][PATCH 01/12] tracing: Have trace_event_file have ref counters Steven Rostedt
@ 2023-11-01 21:37 ` Steven Rostedt
2023-11-01 21:37 ` [for-next][PATCH 05/12] eventfs: Remove "is_freed" union with rcu head Steven Rostedt
` (2 subsequent siblings)
4 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2023-11-01 21:37 UTC (permalink / raw)
To: linux-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, stable,
kernel test robot
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
A synthetic event is created by the synthetic event interface that can
read both user or kernel address memory. In reality, it reads any
arbitrary memory location from within the kernel. If the address space is
in USER (where CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE is set) then
it uses strncpy_from_user_nofault() to copy strings otherwise it uses
strncpy_from_kernel_nofault().
But since both functions use the same variable there's no annotation to
what that variable is (ie. __user). This makes sparse complain.
Quiet sparse by typecasting the strncpy_from_user_nofault() variable to
a __user pointer.
Link: https://lore.kernel.org/linux-trace-kernel/20231031151033.73c42e23@gandalf.local.home
Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Fixes: 0934ae9977c2 ("tracing: Fix reading strings from synthetic events");
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311010013.fm8WTxa5-lkp@intel.com/
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
kernel/trace/trace_events_synth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 14cb275a0bab..846e02c0fb59 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -452,7 +452,7 @@ static unsigned int trace_string(struct synth_trace_event *entry,
#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
if ((unsigned long)str_val < TASK_SIZE)
- ret = strncpy_from_user_nofault(str_field, str_val, STR_VAR_LEN_MAX);
+ ret = strncpy_from_user_nofault(str_field, (const void __user *)str_val, STR_VAR_LEN_MAX);
else
#endif
ret = strncpy_from_kernel_nofault(str_field, str_val, STR_VAR_LEN_MAX);
--
2.42.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [for-next][PATCH 05/12] eventfs: Remove "is_freed" union with rcu head
[not found] <20231101213718.381015321@goodmis.org>
2023-11-01 21:37 ` [for-next][PATCH 01/12] tracing: Have trace_event_file have ref counters Steven Rostedt
2023-11-01 21:37 ` [for-next][PATCH 03/12] tracing: Have the user copy of synthetic event address use correct context Steven Rostedt
@ 2023-11-01 21:37 ` Steven Rostedt
2023-11-01 21:37 ` [for-next][PATCH 10/12] eventfs: Delete eventfs_inode when the last dentry is freed Steven Rostedt
2023-11-01 21:37 ` [for-next][PATCH 12/12] eventfs: Use simple_recursive_removal() to clean up dentries Steven Rostedt
4 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2023-11-01 21:37 UTC (permalink / raw)
To: linux-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, stable, Ajay Kaher
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The eventfs_inode->is_freed was a union with the rcu_head with the
assumption that when it was on the srcu list the head would contain a
pointer which would make "is_freed" true. But that was a wrong assumption
as the rcu head is a single link list where the last element is NULL.
Instead, split the nr_entries integer so that "is_freed" is one bit and
the nr_entries is the next 31 bits. As there shouldn't be more than 10
(currently there's at most 5 to 7 depending on the config), this should
not be a problem.
Link: https://lkml.kernel.org/r/20231101172649.049758712@goodmis.org
Cc: stable@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ajay Kaher <akaher@vmware.com>
Fixes: 63940449555e7 ("eventfs: Implement eventfs lookup, read, open functions")
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
fs/tracefs/event_inode.c | 2 ++
fs/tracefs/internal.h | 6 +++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 9f612a8f009d..1ce73acf3df0 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -824,6 +824,8 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head,
eventfs_remove_rec(ei_child, head, level + 1);
}
+ ei->is_freed = 1;
+
list_del_rcu(&ei->list);
list_add_tail(&ei->del_list, head);
}
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 64fde9490f52..c7d88aaa949f 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -23,6 +23,7 @@ struct tracefs_inode {
* @d_parent: pointer to the parent's dentry
* @d_children: The array of dentries to represent the files when created
* @data: The private data to pass to the callbacks
+ * @is_freed: Flag set if the eventfs is on its way to be freed
* @nr_entries: The number of items in @entries
*/
struct eventfs_inode {
@@ -38,14 +39,13 @@ struct eventfs_inode {
* Union - used for deletion
* @del_list: list of eventfs_inode to delete
* @rcu: eventfs_inode to delete in RCU
- * @is_freed: node is freed if one of the above is set
*/
union {
struct list_head del_list;
struct rcu_head rcu;
- unsigned long is_freed;
};
- int nr_entries;
+ unsigned int is_freed:1;
+ unsigned int nr_entries:31;
};
static inline struct tracefs_inode *get_tracefs(const struct inode *inode)
--
2.42.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [for-next][PATCH 10/12] eventfs: Delete eventfs_inode when the last dentry is freed
[not found] <20231101213718.381015321@goodmis.org>
` (2 preceding siblings ...)
2023-11-01 21:37 ` [for-next][PATCH 05/12] eventfs: Remove "is_freed" union with rcu head Steven Rostedt
@ 2023-11-01 21:37 ` Steven Rostedt
2023-11-01 21:37 ` [for-next][PATCH 12/12] eventfs: Use simple_recursive_removal() to clean up dentries Steven Rostedt
4 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2023-11-01 21:37 UTC (permalink / raw)
To: linux-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, stable, Ajay Kaher
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
There exists a race between holding a reference of an eventfs_inode dentry
and the freeing of the eventfs_inode. If user space has a dentry held long
enough, it may still be able to access the dentry's eventfs_inode after it
has been freed.
To prevent this, have he eventfs_inode freed via the last dput() (or via
RCU if the eventfs_inode does not have a dentry).
This means reintroducing the eventfs_inode del_list field at a temporary
place to put the eventfs_inode. It needs to mark it as freed (via the
list) but also must invalidate the dentry immediately as the return from
eventfs_remove_dir() expects that they are. But the dentry invalidation
must not be called under the eventfs_mutex, so it must be done after the
eventfs_inode is marked as free (put on a deletion list).
Link: https://lkml.kernel.org/r/20231101172650.123479767@goodmis.org
Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ajay Kaher <akaher@vmware.com>
Fixes: 5bdcd5f5331a2 ("eventfs: Implement removal of meta data from eventfs")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
fs/tracefs/event_inode.c | 146 ++++++++++++++++++---------------------
fs/tracefs/internal.h | 2 +
2 files changed, 69 insertions(+), 79 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 8ac9abf7a3d5..0a04ae0ca8c8 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -85,8 +85,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
mutex_lock(&eventfs_mutex);
ei = dentry->d_fsdata;
- /* The LSB is set when the eventfs_inode is being freed */
- if (((unsigned long)ei & 1UL) || ei->is_freed) {
+ if (ei->is_freed) {
/* Do not allow changes if the event is about to be removed. */
mutex_unlock(&eventfs_mutex);
return -ENODEV;
@@ -276,35 +275,17 @@ static void free_ei(struct eventfs_inode *ei)
void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry)
{
struct tracefs_inode *ti_parent;
- struct eventfs_inode *ei_child, *tmp;
struct eventfs_inode *ei;
int i;
/* The top level events directory may be freed by this */
if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
- LIST_HEAD(ef_del_list);
-
mutex_lock(&eventfs_mutex);
-
ei = ti->private;
-
- /* Record all the top level files */
- list_for_each_entry_srcu(ei_child, &ei->children, list,
- lockdep_is_held(&eventfs_mutex)) {
- list_add_tail(&ei_child->del_list, &ef_del_list);
- }
-
/* Nothing should access this, but just in case! */
ti->private = NULL;
-
mutex_unlock(&eventfs_mutex);
- /* Now safely free the top level files and their children */
- list_for_each_entry_safe(ei_child, tmp, &ef_del_list, del_list) {
- list_del(&ei_child->del_list);
- eventfs_remove_dir(ei_child);
- }
-
free_ei(ei);
return;
}
@@ -319,14 +300,6 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry)
if (!ei)
goto out;
- /*
- * If ei was freed, then the LSB bit is set for d_fsdata.
- * But this should not happen, as it should still have a
- * ref count that prevents it. Warn in case it does.
- */
- if (WARN_ON_ONCE((unsigned long)ei & 1))
- goto out;
-
/* This could belong to one of the files of the ei */
if (ei->dentry != dentry) {
for (i = 0; i < ei->nr_entries; i++) {
@@ -336,6 +309,8 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry)
if (WARN_ON_ONCE(i == ei->nr_entries))
goto out;
ei->d_children[i] = NULL;
+ } else if (ei->is_freed) {
+ free_ei(ei);
} else {
ei->dentry = NULL;
}
@@ -962,13 +937,65 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
return ERR_PTR(-ENOMEM);
}
+static LLIST_HEAD(free_list);
+
+static void eventfs_workfn(struct work_struct *work)
+{
+ struct eventfs_inode *ei, *tmp;
+ struct llist_node *llnode;
+
+ llnode = llist_del_all(&free_list);
+ llist_for_each_entry_safe(ei, tmp, llnode, llist) {
+ /* This dput() matches the dget() from unhook_dentry() */
+ for (int i = 0; i < ei->nr_entries; i++) {
+ if (ei->d_children[i])
+ dput(ei->d_children[i]);
+ }
+ /* This should only get here if it had a dentry */
+ if (!WARN_ON_ONCE(!ei->dentry))
+ dput(ei->dentry);
+ }
+}
+
+static DECLARE_WORK(eventfs_work, eventfs_workfn);
+
static void free_rcu_ei(struct rcu_head *head)
{
struct eventfs_inode *ei = container_of(head, struct eventfs_inode, rcu);
+ if (ei->dentry) {
+ /* Do not free the ei until all references of dentry are gone */
+ if (llist_add(&ei->llist, &free_list))
+ queue_work(system_unbound_wq, &eventfs_work);
+ return;
+ }
+
+ /* If the ei doesn't have a dentry, neither should its children */
+ for (int i = 0; i < ei->nr_entries; i++) {
+ WARN_ON_ONCE(ei->d_children[i]);
+ }
+
free_ei(ei);
}
+static void unhook_dentry(struct dentry *dentry)
+{
+ if (!dentry)
+ return;
+
+ /* Keep the dentry from being freed yet (see eventfs_workfn()) */
+ dget(dentry);
+
+ dentry->d_fsdata = NULL;
+ d_invalidate(dentry);
+ mutex_lock(&eventfs_mutex);
+ /* dentry should now have at least a single reference */
+ WARN_ONCE((int)d_count(dentry) < 1,
+ "dentry %px (%s) less than one reference (%d) after invalidate\n",
+ dentry, dentry->d_name.name, d_count(dentry));
+ mutex_unlock(&eventfs_mutex);
+}
+
/**
* eventfs_remove_rec - remove eventfs dir or file from list
* @ei: eventfs_inode to be removed.
@@ -1006,33 +1033,6 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head,
list_add_tail(&ei->del_list, head);
}
-static void unhook_dentry(struct dentry **dentry, struct dentry **list)
-{
- if (*dentry) {
- unsigned long ptr = (unsigned long)*list;
-
- /* Keep the dentry from being freed yet */
- dget(*dentry);
-
- /*
- * Paranoid: The dget() above should prevent the dentry
- * from being freed and calling eventfs_set_ei_status_free().
- * But just in case, set the link list LSB pointer to 1
- * and have eventfs_set_ei_status_free() check that to
- * make sure that if it does happen, it will not think
- * the d_fsdata is an eventfs_inode.
- *
- * For this to work, no eventfs_inode should be allocated
- * on a odd space, as the ef should always be allocated
- * to be at least word aligned. Check for that too.
- */
- WARN_ON_ONCE(ptr & 1);
-
- (*dentry)->d_fsdata = (void *)(ptr | 1);
- *list = *dentry;
- *dentry = NULL;
- }
-}
/**
* eventfs_remove_dir - remove eventfs dir or file from list
* @ei: eventfs_inode to be removed.
@@ -1043,40 +1043,28 @@ void eventfs_remove_dir(struct eventfs_inode *ei)
{
struct eventfs_inode *tmp;
LIST_HEAD(ei_del_list);
- struct dentry *dentry_list = NULL;
- struct dentry *dentry;
- int i;
if (!ei)
return;
+ /*
+ * Move the deleted eventfs_inodes onto the ei_del_list
+ * which will also set the is_freed value. Note, this has to be
+ * done under the eventfs_mutex, but the deletions of
+ * the dentries must be done outside the eventfs_mutex.
+ * Hence moving them to this temporary list.
+ */
mutex_lock(&eventfs_mutex);
eventfs_remove_rec(ei, &ei_del_list, 0);
+ mutex_unlock(&eventfs_mutex);
list_for_each_entry_safe(ei, tmp, &ei_del_list, del_list) {
- for (i = 0; i < ei->nr_entries; i++)
- unhook_dentry(&ei->d_children[i], &dentry_list);
- unhook_dentry(&ei->dentry, &dentry_list);
+ for (int i = 0; i < ei->nr_entries; i++)
+ unhook_dentry(ei->d_children[i]);
+ unhook_dentry(ei->dentry);
+ list_del(&ei->del_list);
call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei);
}
- mutex_unlock(&eventfs_mutex);
-
- while (dentry_list) {
- unsigned long ptr;
-
- dentry = dentry_list;
- ptr = (unsigned long)dentry->d_fsdata & ~1UL;
- dentry_list = (struct dentry *)ptr;
- dentry->d_fsdata = NULL;
- d_invalidate(dentry);
- mutex_lock(&eventfs_mutex);
- /* dentry should now have at least a single reference */
- WARN_ONCE((int)d_count(dentry) < 1,
- "dentry %px (%s) less than one reference (%d) after invalidate\n",
- dentry, dentry->d_name.name, d_count(dentry));
- mutex_unlock(&eventfs_mutex);
- dput(dentry);
- }
}
/**
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 5f60bcd69289..06a1f220b901 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -54,10 +54,12 @@ struct eventfs_inode {
void *data;
/*
* Union - used for deletion
+ * @llist: for calling dput() if needed after RCU
* @del_list: list of eventfs_inode to delete
* @rcu: eventfs_inode to delete in RCU
*/
union {
+ struct llist_node llist;
struct list_head del_list;
struct rcu_head rcu;
};
--
2.42.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [for-next][PATCH 12/12] eventfs: Use simple_recursive_removal() to clean up dentries
[not found] <20231101213718.381015321@goodmis.org>
` (3 preceding siblings ...)
2023-11-01 21:37 ` [for-next][PATCH 10/12] eventfs: Delete eventfs_inode when the last dentry is freed Steven Rostedt
@ 2023-11-01 21:37 ` Steven Rostedt
4 siblings, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2023-11-01 21:37 UTC (permalink / raw)
To: linux-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, stable, Al Viro
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Looking at how dentry is removed via the tracefs system, I found that
eventfs does not do everything that it did under tracefs. The tracefs
removal of a dentry calls simple_recursive_removal() that does a lot more
than a simple d_invalidate().
As it should be a requirement that any eventfs_inode that has a dentry, so
does its parent. When removing a eventfs_inode, if it has a dentry, a call
to simple_recursive_removal() on that dentry should clean up all the
dentries underneath it.
Add WARN_ON_ONCE() to check for the parent having a dentry if any children
do.
Link: https://lore.kernel.org/all/20231101022553.GE1957730@ZenIV/
Link: https://lkml.kernel.org/r/20231101172650.552471568@goodmis.org
Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Fixes: 5bdcd5f5331a2 ("eventfs: Implement removal of meta data from eventfs")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
fs/tracefs/event_inode.c | 77 +++++++++++++++++++++++-----------------
fs/tracefs/internal.h | 2 --
2 files changed, 44 insertions(+), 35 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 0087a3f455f1..f8a594a50ae6 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -967,30 +967,29 @@ static void unhook_dentry(struct dentry *dentry)
{
if (!dentry)
return;
-
- /* Keep the dentry from being freed yet (see eventfs_workfn()) */
+ /*
+ * Need to add a reference to the dentry that is expected by
+ * simple_recursive_removal(), which will include a dput().
+ */
dget(dentry);
- dentry->d_fsdata = NULL;
- d_invalidate(dentry);
- mutex_lock(&eventfs_mutex);
- /* dentry should now have at least a single reference */
- WARN_ONCE((int)d_count(dentry) < 1,
- "dentry %px (%s) less than one reference (%d) after invalidate\n",
- dentry, dentry->d_name.name, d_count(dentry));
- mutex_unlock(&eventfs_mutex);
+ /*
+ * Also add a reference for the dput() in eventfs_workfn().
+ * That is required as that dput() will free the ei after
+ * the SRCU grace period is over.
+ */
+ dget(dentry);
}
/**
* eventfs_remove_rec - remove eventfs dir or file from list
* @ei: eventfs_inode to be removed.
- * @head: the list head to place the deleted @ei and children
* @level: prevent recursion from going more than 3 levels deep.
*
* This function recursively removes eventfs_inodes which
* contains info of files and/or directories.
*/
-static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, int level)
+static void eventfs_remove_rec(struct eventfs_inode *ei, int level)
{
struct eventfs_inode *ei_child;
@@ -1009,13 +1008,26 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head,
/* search for nested folders or files */
list_for_each_entry_srcu(ei_child, &ei->children, list,
lockdep_is_held(&eventfs_mutex)) {
- eventfs_remove_rec(ei_child, head, level + 1);
+ /* Children only have dentry if parent does */
+ WARN_ON_ONCE(ei_child->dentry && !ei->dentry);
+ eventfs_remove_rec(ei_child, level + 1);
}
+
ei->is_freed = 1;
+ for (int i = 0; i < ei->nr_entries; i++) {
+ if (ei->d_children[i]) {
+ /* Children only have dentry if parent does */
+ WARN_ON_ONCE(!ei->dentry);
+ unhook_dentry(ei->d_children[i]);
+ }
+ }
+
+ unhook_dentry(ei->dentry);
+
list_del_rcu(&ei->list);
- list_add_tail(&ei->del_list, head);
+ call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei);
}
/**
@@ -1026,30 +1038,22 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head,
*/
void eventfs_remove_dir(struct eventfs_inode *ei)
{
- struct eventfs_inode *tmp;
- LIST_HEAD(ei_del_list);
+ struct dentry *dentry;
if (!ei)
return;
- /*
- * Move the deleted eventfs_inodes onto the ei_del_list
- * which will also set the is_freed value. Note, this has to be
- * done under the eventfs_mutex, but the deletions of
- * the dentries must be done outside the eventfs_mutex.
- * Hence moving them to this temporary list.
- */
mutex_lock(&eventfs_mutex);
- eventfs_remove_rec(ei, &ei_del_list, 0);
+ dentry = ei->dentry;
+ eventfs_remove_rec(ei, 0);
mutex_unlock(&eventfs_mutex);
- list_for_each_entry_safe(ei, tmp, &ei_del_list, del_list) {
- for (int i = 0; i < ei->nr_entries; i++)
- unhook_dentry(ei->d_children[i]);
- unhook_dentry(ei->dentry);
- list_del(&ei->del_list);
- call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei);
- }
+ /*
+ * If any of the ei children has a dentry, then the ei itself
+ * must have a dentry.
+ */
+ if (dentry)
+ simple_recursive_removal(dentry, NULL);
}
/**
@@ -1060,10 +1064,17 @@ void eventfs_remove_dir(struct eventfs_inode *ei)
*/
void eventfs_remove_events_dir(struct eventfs_inode *ei)
{
- struct dentry *dentry = ei->dentry;
+ struct dentry *dentry;
+ dentry = ei->dentry;
eventfs_remove_dir(ei);
- /* Matches the dget() from eventfs_create_events_dir() */
+ /*
+ * Matches the dget() done by tracefs_start_creating()
+ * in eventfs_create_events_dir() when it the dentry was
+ * created. In other words, it's a normal dentry that
+ * sticks around while the other ei->dentry are created
+ * and destroyed dynamically.
+ */
dput(dentry);
}
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 06a1f220b901..ccee18ca66c7 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -55,12 +55,10 @@ struct eventfs_inode {
/*
* Union - used for deletion
* @llist: for calling dput() if needed after RCU
- * @del_list: list of eventfs_inode to delete
* @rcu: eventfs_inode to delete in RCU
*/
union {
struct llist_node llist;
- struct list_head del_list;
struct rcu_head rcu;
};
unsigned int is_freed:1;
--
2.42.0
^ permalink raw reply related [flat|nested] 5+ messages in thread